-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add OC_API annotations to oc_buffer_settings.h #647
Conversation
WalkthroughThis pull request introduces several changes across the IoTivity-lite codebase, focusing on network monitoring, buffer settings, and API enhancements. Key modifications include the addition of new functions for managing buffer settings, conditional compilation for network monitoring features, updates to session and network interface event handling, and improvements to string and enum utility functions. The changes aim to enhance flexibility, safety, and modularity within the IoTivity-lite library by providing more granular control over system configurations and event handling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant BufferSettings
participant NetworkMonitor
Client->>BufferSettings: Set MTU Size
BufferSettings-->>Client: Return Status
Client->>BufferSettings: Set Max App Data Size
BufferSettings-->>Client: Return Status
Client->>NetworkMonitor: Add Network Interface Callback
NetworkMonitor-->>Client: Callback Registration Status
Client->>NetworkMonitor: Remove Network Interface Callback
NetworkMonitor-->>Client: Callback Removal Status
This sequence diagram illustrates the high-level interactions for buffer settings configuration and network interface event callback management, showcasing the new functionalities introduced in the pull request. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Thank you for your code contribution! To guarantee the change/addition is conformant to the OCF Specification, we would like to ask you to execute OCF Conformance Testing of your change ☝️ when your work is ready to be reviewed. ℹ️ To verify your latest change (e4b351b), label this PR with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
include/oc_buffer_settings.h (1)
87-88
: Add documentation for the new oc_get_block_size function.The addition of the
oc_get_block_size
function with the OC_API annotation is good. However, unlike other functions in this file, it's missing documentation. Please add a documentation comment explaining the purpose of this function and what the returned block size represents.Consider adding documentation similar to other functions in this file:
/** * @brief retrieve the block size * * @return long the block size in bytes */ OC_API long oc_get_block_size(void);api/oc_buffer_settings.c (2)
105-110
: Simplify the conditional compilation inoc_set_min_app_data_size
.The preprocessor condition:
#if defined(OC_APP_DATA_BUFFER_SIZE) || !defined(OC_REP_ENCODING_REALLOC)results in the function doing nothing when either
OC_APP_DATA_BUFFER_SIZE
is defined orOC_REP_ENCODING_REALLOC
is not defined. This might be confusing. Consider revising the condition or adding comments to clarify under which configurations the function has an effect.
1-17
: Ensure correct license and documentation comments.The header comment appears standard, but double-check that it complies with the project's licensing requirements and that all necessary attributions are included.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- api/oc_buffer_settings.c (1 hunks)
- api/oc_main.c (0 hunks)
- include/oc_buffer_settings.h (3 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- api/oc_main.c
🔇 Additional comments (12)
include/oc_buffer_settings.h (8)
24-25
: LGTM: Include statement for OC_API macro added.The addition of
#include "oc_export.h"
is appropriate as it likely defines the OC_API macro used in the function declarations below. This change aligns with the PR objective of adding OC_API annotations.
38-39
: LGTM: OC_API annotation added to oc_set_mtu_size.The addition of the OC_API macro to the
oc_set_mtu_size
function declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
46-47
: LGTM: OC_API annotation added to oc_get_mtu_size.The addition of the OC_API macro to the
oc_get_mtu_size
function declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
54-55
: LGTM: OC_API annotation added to oc_set_max_app_data_size.The addition of the OC_API macro to the
oc_set_max_app_data_size
function declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
62-63
: LGTM: OC_API annotation added to oc_get_max_app_data_size.The addition of the OC_API macro to the
oc_get_max_app_data_size
function declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
71-72
: LGTM: OC_API annotation added to oc_set_min_app_data_size.The addition of the OC_API macro to the
oc_set_min_app_data_size
function declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
79-80
: LGTM: OC_API annotation added to oc_get_min_app_data_size.The addition of the OC_API macro to the
oc_get_min_app_data_size
function declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
Line range hint
1-93
: Overall assessment: Changes look good with a minor improvement needed.The changes in this file consistently add OC_API annotations to the existing functions, which aligns well with the PR objective. The addition of the new
oc_get_block_size
function is also good, but it needs documentation to maintain consistency with other functions in the file.Here's a summary of the changes:
- Added include for "oc_export.h"
- Added OC_API annotations to all existing functions
- Added a new function
oc_get_block_size
with OC_API annotationOnce the documentation for
oc_get_block_size
is added, this file will be in excellent shape.api/oc_buffer_settings.c (4)
30-33
: Ensure consistent default MTU size calculation.In the
#else
block forOC_INOUT_BUFFER_SIZE
, the default_OC_MTU_SIZE
is set to2048 + COAP_MAX_HEADER_SIZE
. Verify that this default value aligns with the expected MTU size in the absence ofOC_INOUT_BUFFER_SIZE
. If2048
is meant to be a placeholder, consider defining it as a macro or explaining its derivation for clarity.
80-94
: Ensure correct handling inoc_set_max_app_data_size
.In the function
oc_set_max_app_data_size
:
- When
OC_APP_DATA_BUFFER_SIZE
is not defined,_OC_MAX_APP_DATA_SIZE
is updated with the providedsize
.- If
OC_REP_ENCODING_REALLOC
is not defined,_OC_MIN_APP_DATA_SIZE
is also updated, ensuring consistency between max and min sizes.Please confirm that this logic accurately reflects the intended behavior, especially in different configuration scenarios.
19-24
: Include necessary headers for complete functionality.Ensure that all required headers are included, especially those related to the types used (e.g.,
size_t
) and any external dependencies. For example, ifCOAP_MAX_HEADER_SIZE
is defined elsewhere, verify that its header is included.Run the following script to check for required includes:
✅ Verification successful
To gather the necessary information, I'll need to examine the contents of the api/oc_buffer_settings.c file and search for any usage of COAP_MAX_HEADER_SIZE. Here's the script to do that:
The includes in api/oc_buffer_settings.c are correct and complete.
The file includes all necessary headers for the types and functions it uses. The COAP_MAX_HEADER_SIZE constant is correctly included via "messaging/coap/conf.h" inside a conditional block, which matches its conditional usage in the code. This design choice reduces unnecessary includes when dynamic allocation is not used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for necessary header files. # List all include statements. rg --type c "^#include" # Search for the definition of COAP_MAX_HEADER_SIZE. rg --type c "define COAP_MAX_HEADER_SIZE"Length of output: 246276
Script:
#!/bin/bash # Search for COAP_MAX_HEADER_SIZE usage in api/oc_buffer_settings.c echo "Searching for COAP_MAX_HEADER_SIZE usage:" grep -n "COAP_MAX_HEADER_SIZE" api/oc_buffer_settings.c # Display the contents of api/oc_buffer_settings.c echo -e "\nContents of api/oc_buffer_settings.c:" cat api/oc_buffer_settings.cLength of output: 4587
48-71
: Validate MTU size adjustment logic inoc_set_mtu_size
.In the function
oc_set_mtu_size
, there is conditional logic that adjusts_OC_MTU_SIZE
and_OC_BLOCK_SIZE
based on the providedmtu_size
. Please verify:
- The check
if (mtu_size < (COAP_MAX_HEADER_SIZE + 16))
ensures the MTU size is sufficient. Confirm that the16
byte overhead is appropriate.- The loop calculating
_OC_BLOCK_SIZE
uses bit shifting to find the highest power of two less thanmtu_size
. Ensure this correctly derives the desired block size.- When
OC_OSCORE
is defined,_OC_MTU_SIZE
is adjusted differently. Verify that this aligns with the security requirements.Run the following script to check for consistent usage of
COAP_MAX_HEADER_SIZE
and calculations of MTU and block sizes:✅ Verification successful
MTU Size Adjustment Logic Verified.
- The
16
byte overhead inif (mtu_size < (COAP_MAX_HEADER_SIZE + 16))
is consistently enforced through both compile-time and run-time checks, ensuring compliance with protocol specifications.- The bit-shifting loop accurately calculates the highest power of two less than
mtu_size
, correctly determining_OC_BLOCK_SIZE
.- When
OC_OSCORE
is defined,_OC_MTU_SIZE
is appropriately adjusted to includeCOAP_MAX_HEADER_SIZE
, aligning with security requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check calculations of MTU and block sizes. # Search for all instances where MTU and block sizes are calculated. rg --type c "_OC_MTU_SIZE|_OC_BLOCK_SIZE|COAP_MAX_HEADER_SIZE"Length of output: 5253
Script:
#!/bin/bash # Description: Validate the 16-byte overhead and inspect the loop logic in oc_set_mtu_size. # Find where the 16-byte overhead is defined or used in relation to COAP_MAX_HEADER_SIZE rg --type c 'COAP_MAX_HEADER_SIZE\s*\+\s*16' # Extract the implementation of the oc_set_mtu_size function to inspect the loop logic ast-grep --lang c --pattern $'void oc_set_mtu_size($_) { $$$ }' api/oc_buffer_settings.cLength of output: 288
Script:
#!/bin/bash # Description: Extract the oc_set_mtu_size function with a flexible pattern and further investigate related calculations. # Attempt to extract the oc_set_mtu_size function with a relaxed pattern to account for different signatures ast-grep --lang c --pattern $'void oc_set_mtu_size($_) { $$$ }' api/oc_buffer_settings.c # If ast-grep fails, use ripgrep to locate the function definition rg --type c 'void\s+oc_set_mtu_size\s*\(' api/oc_buffer_settings.c # Investigate the source or documentation for the 16-byte overhead to understand its rationale rg --type c 'COAP_MAX_HEADER_SIZE\s*\+\s*16' -C 5 # Check definitions related to OC_OSCORE to verify security alignment rg --type c 'OC_OSCORE' --context 3Length of output: 49198
Script:
#!/bin/bash # Description: Extract the oc_set_mtu_size function with a flexible pattern and gather detailed context. # Attempt to extract the oc_set_mtu_size function with a flexible pattern to account for different return types and qualifiers ast-grep --lang c --pattern $'$_ oc_set_mtu_size($_) { $$$ }' api/oc_buffer_settings.c # If ast-grep fails, use ripgrep to locate the function definition and extract surrounding lines for context rg --type c 'oc_set_mtu_size\s*\(' api/oc_buffer_settings.c -A 20 -B 10 # Trace the origin of the 16-byte overhead to understand its rationale rg --type c 'COAP_MAX_HEADER_SIZE' api/oc_buffer_settings.c port/oc_connectivity.h -C 5 # Inspect the definitions and usages of OC_OSCORE to verify alignment with security requirements rg --type c 'OC_OSCORE' api/oc_buffer_settings.c port/oc_connectivity.h security/oc_oscore_engine.c security/oc_obt.c security/oc_tls.cLength of output: 9003
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
include/oc_enums.h (1)
237-239
: LGTM: Add OC_API to oc_str_to_enum_locn function. Minor documentation improvement suggested.The addition of the OC_API macro to the
oc_str_to_enum_locn
function declaration is correct and aligns with the PR objective. This change appropriately exposes the function as part of the public API.Consider updating the function's documentation to reflect that the
oc_defined
parameter is non-null. This can be done by changing:* @param oc_defined value to check if the conversion is successfull (cannot be * NULL)to:
* @param oc_defined [out] Non-null pointer to a boolean that will be set to indicate if the conversion was successfulThis change would make the documentation more consistent with the
OC_NONNULL
attribute and provide clearer information about the parameter's usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- include/oc_enums.h (5 hunks)
- port/esp32/main/CMakeLists.txt (1 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj (1 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj.filters (1 hunks)
- swig/swig_interfaces/oc_buffer_settings.i (1 hunks)
- swig/swig_interfaces/oc_enums.i (1 hunks)
🔇 Additional comments (10)
swig/swig_interfaces/oc_buffer_settings.i (2)
25-26
: LGTM: OC_API definition added correctlyThe addition of
#define OC_API
before including theoc_buffer_settings.h
header is correct and aligns with the PR objective of adding OC_API annotations.
Line range hint
18-22
: Verify function renames mentioned in the AI summaryThe AI summary mentions renaming of 'getMaxAppDataSize' to 'oc_get_max_app_data_size' and 'getBlockSize' to 'oc_get_block_size'. However, the existing renames in the code seem to be in the opposite direction. Please verify if any changes are needed here.
Let's check for any missed renames or inconsistencies:
swig/swig_interfaces/oc_enums.i (1)
35-35
: Consider the necessity and impact ofOC_API
in SWIG interfaceThe addition of
#define OC_API
seems to be related to API visibility in the C/C++ code. However, in the context of a SWIG interface file, this definition might not have the intended effect or be necessary.To ensure this change is consistent with other SWIG interface files and doesn't introduce any issues, please run the following script:
This script will help us understand if
OC_API
is consistently used across SWIG interfaces and if it's actually used in theoc_enums.h
header or other related headers.✅ Verification successful
OC_API definition is consistent across SWIG interface files
The addition of
#define OC_API
inswig/swig_interfaces/oc_enums.i
aligns with its usage across other SWIG interface and header files. No issues detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for OC_API usage in other SWIG interface files # Test 1: Check if OC_API is defined in other SWIG interface files echo "Checking for OC_API in other SWIG interface files:" rg --type swig '#define\s+OC_API' swig/ # Test 2: Check if OC_API is used in the oc_enums.h header echo "Checking for OC_API usage in oc_enums.h:" rg --type c 'OC_API' include/oc_enums.h # Test 3: Check if OC_API is used in other header files echo "Checking for OC_API usage in other header files:" rg --type c 'OC_API' include/Length of output: 12247
include/oc_enums.h (5)
30-30
: LGTM: Include "oc_export.h" for OC_API macro.The addition of
#include "oc_export.h"
is appropriate and necessary for the OC_API macro used in the function declarations below.
208-209
: LGTM: Add OC_API to oc_enum_to_str function.The addition of the OC_API macro to the
oc_enum_to_str
function declaration is correct and aligns with the PR objective. This change appropriately exposes the function as part of the public API.
217-218
: LGTM: Add OC_API to oc_enum_pos_desc_to_str function.The addition of the OC_API macro to the
oc_enum_pos_desc_to_str
function declaration is correct and aligns with the PR objective. This change appropriately exposes the function as part of the public API.
226-227
: LGTM: Add OC_API to oc_enum_locn_to_str function.The addition of the OC_API macro to the
oc_enum_locn_to_str
function declaration is correct and aligns with the PR objective. This change appropriately exposes the function as part of the public API.
Line range hint
1-262
: Overall assessment: Changes look good and align with PR objectives.The modifications to
include/oc_enums.h
consistently add the OC_API macro to the relevant function declarations, which aligns with the PR objective of adding OC_API annotations. These changes appropriately expose the functions as part of the public API, improving their visibility and linkage.The inclusion of "oc_export.h" is correct and necessary for the OC_API macro definition. All changes are consistent and do not introduce any issues that would prevent merging this PR.
Great job on improving the API visibility of these functions!
port/windows/vs2015/IoTivity-lite.vcxproj (1)
357-357
: LGTM: New source file correctly added to the project.The addition of
oc_buffer_settings.c
to the project file is correct and aligns with the PR objectives. This ensures that the new functionality related to buffer settings will be compiled and linked properly.port/windows/vs2015/IoTivity-lite.vcxproj.filters (1)
116-118
: LGTM: New file added correctly to the project.The addition of
oc_buffer_settings.c
to the project is correct and consistent with the existing structure. It's appropriately placed in the "Core" filter, which aligns with its likely functionality as a core component of the IoTivity-lite project.To ensure this file exists and is in the correct location, run the following script:
✅ Verification successful
Verification Successful:
oc_buffer_settings.c
is correctly added to the project.The file exists in the expected location and is appropriately placed within the "Core" filter. No issues detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and location of oc_buffer_settings.c # Check if the file exists in the expected location if fd -p "api/oc_buffer_settings.c" ; then echo "File oc_buffer_settings.c found in the correct location." else echo "Error: oc_buffer_settings.c not found in the expected location." fi # Check if there are any other files with similar names that might have been misnamed fd -p "oc_buffer_settings.*\.c"Length of output: 239
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (15)
swig/swig_interfaces/oc_session_events.i (1)
42-42
: LGTM! Consider adding documentation for the new macro.The addition of the
OC_DEPRECATED
macro is a good preparation for managing deprecated APIs in the future. This change doesn't affect the current functionality but provides a tool for better API lifecycle management.Consider adding a comment above the macro definition to explain its purpose and usage. For example:
// Macro to mark deprecated functions or methods. Usage: OC_DEPRECATED(message) before the function declaration. #define OC_DEPRECATED(...)include/oc_network_monitor.h (2)
49-49
: LGTM: New function added with correct annotationThe addition of the
oc_remove_network_interface_event_callback
function is a good improvement, providing symmetry with the existing add function. It's correctly annotated withOC_API
and has a consistent signature with the add function.Consider updating the comment above the function to include the same level of detail as the add function, specifically mentioning that the callback must not be NULL:
/** * @brief Remove the callback to receive change notifications for Network * interface event. * @param cb The callback to be removed. Must not be NULL. * @return 0 on success * @return -1 on error */
Detected usages of removed functions in the codebase
Several parts of the codebase still use the removed functions
oc_add_session_event_callback
andoc_remove_session_event_callback
. These usages could lead to compatibility issues.Key locations:
port/unittest/connectivitytest.cpp
:
- Multiple test cases using
oc_add_session_event_callback
andoc_remove_session_event_callback
.include/oc_session_events.h
:
- Deprecated function declarations still present.
api/oc_session_events.c
:
- Implementations of deprecated functions.
api/plgd/unittest/plgdtimetest.cpp
:
- Test cases invoking deprecated functions.
- Other ports and API implementations also reference these deprecated functions.
🔗 Analysis chain
Line range hint
1-53
: Verify impact of API changes and update documentationThe changes to this header file represent a significant update to the network monitoring API:
- Removal of deprecated session event callback functions.
- Clear marking of public API functions with
OC_API
.- Addition of a new function to remove network interface event callbacks.
These changes improve the API's clarity and completeness. However, they may break backwards compatibility for any code still using the removed functions.
Please run the following script to check for any remaining uses of the removed functions in the codebase:
Also, ensure that the API documentation (if separate from this header) is updated to reflect these changes, including the removal of deprecated functions and the addition of the new remove function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for uses of removed functions in the codebase echo "Checking for uses of removed functions:" rg --type c --type cpp "oc_add_session_event_callback(_v1)?" || echo "No uses of oc_add_session_event_callback found" rg --type c --type cpp "oc_remove_session_event_callback(_v1)?" || echo "No uses of oc_remove_session_event_callback found" echo "Checking for potential uses of new function:" rg --type c --type cpp "oc_remove_network_interface_event_callback" || echo "No uses of oc_remove_network_interface_event_callback found"Length of output: 7999
port/oc_storage.h (1)
47-53
: LGTM: OC_API and OC_NONNULL added to oc_storage_readThe changes to oc_storage_read are well-implemented:
- The OC_API annotation correctly marks this function as part of the public API.
- The OC_NONNULL() attribute enhances safety by enforcing non-null parameters.
- The updated comments clearly indicate which parameters cannot be NULL.
These modifications improve both the usability and safety of the function.
Consider updating the return value comment to clarify the behavior on error:
- * @return long amount of bytes read + * @return long amount of bytes read on success, or a negative value on errorapi/unittest/buffersettingstest.cpp (4)
25-37
: Consider adding more test cases for SetMTUSize.The current test case checks for an invalid MTU size, which is good. However, to ensure comprehensive coverage, consider adding the following test cases:
- Setting a valid MTU size and verifying it's set correctly.
- Setting the minimum allowed MTU size.
- Setting the maximum allowed MTU size.
This will help ensure that the function behaves correctly across its entire range of valid inputs.
39-46
: Enhance the SetMaxAppDataSize test case.While the current test case covers basic functionality, consider the following improvements:
- Test setting the maximum allowed app data size.
- Test setting a size larger than the maximum allowed and verify it fails or is capped.
- Test setting the minimum allowed app data size.
- Verify that setting an invalid size (e.g., 0 or a negative value) is handled correctly.
These additions will provide more comprehensive coverage of the
oc_set_max_app_data_size
function's behavior.
62-85
: Improve test descriptions for non-dynamic allocation scenarios.The current tests effectively check the behavior when dynamic allocation is disabled. To enhance clarity and maintainability, consider the following improvements:
- Add comments or update the test names to explain why -1 is the expected result in each case.
- For the
GetBlockSize
test, explain the significance of the -1 return value.Example:
TEST(BufferSettings, SetMTUSize_ReturnsNegativeOneWhenDynamicAllocationDisabled) { // When dynamic allocation is disabled, setting MTU size should not be possible EXPECT_EQ(-1, oc_set_mtu_size(42)); EXPECT_EQ(-1, oc_get_mtu_size()); }This will make the tests more self-documenting and easier to understand for future maintainers.
1-87
: Consider enhancing overall test structure and coverage.The test file provides good coverage for different compilation scenarios. To further improve it, consider the following suggestions:
- Add a test fixture (e.g.,
BufferSettingsTest
) to manage common setup and teardown operations, if any.- Group related tests using test suites (e.g.,
TEST_F(BufferSettingsTest, DynamicAllocation)
andTEST_F(BufferSettingsTest, StaticAllocation)
).- Add edge case tests, such as testing with maximum and minimum valid values for each setting.
- Consider adding tests for any error logging or error handling mechanisms in the buffer settings functions.
- If possible, add tests that verify the actual impact of these settings on the buffer behavior in the system.
These improvements will enhance the overall quality and maintainability of the test suite.
include/oc_session_events.h (3)
80-91
: LGTM: New function addition with proper deprecation noticeThe addition of
oc_add_session_event_callback
with a clear deprecation notice is well-implemented. The function signature, documentation, and return values are clearly defined.Consider adding a brief explanation in the documentation about why this function is deprecated and the benefits of using the new
v1
version. This can help users understand the motivation behind the change and encourage them to migrate to the new function.
93-103
: LGTM: New v1 function with improved flexibilityThe addition of
oc_add_session_event_callback_v1
with the extrauser_data
parameter is a good improvement. It provides more flexibility for users and follows a common pattern for callback functions.Consider adding a brief example in the documentation showing how to use this function, especially demonstrating the use of the
user_data
parameter. This can help users quickly understand how to migrate from the deprecated version to this new one.
117-132
: LGTM: New v1 removal function with improved flexibilityThe addition of
oc_remove_session_event_callback_v1
with the extrauser_data
andignore_user_data
parameters is a good improvement. It provides more flexibility for users when removing callbacks and follows a consistent pattern with the add function.Consider adding a brief explanation or example in the documentation showing how the
ignore_user_data
parameter affects the behavior of the function. This can help users understand when and how to use this parameter effectively.api/plgd/device-provisioning-client/plgd_dps.c (1)
232-242
: LGTM: Conditional network monitoring callback registrationThe changes to
plgd_dps_interface_callbacks_init
andplgd_dps_interface_callbacks_deinit
are consistent with the conditional inclusion of the network monitoring header. This ensures that network monitoring functionality is only active when OC_NETWORK_MONITOR is defined, allowing for better control over feature inclusion.For consistency, consider adding a comment to explain why these functions are empty when OC_NETWORK_MONITOR is not defined. For example:
void plgd_dps_interface_callbacks_init(void) { +#ifndef OC_NETWORK_MONITOR + // No-op when network monitoring is disabled +#else oc_add_network_interface_event_callback(dps_interface_event_handler); #endif /* OC_NETWORK_MONITOR */ } void plgd_dps_interface_callbacks_deinit(void) { +#ifndef OC_NETWORK_MONITOR + // No-op when network monitoring is disabled +#else oc_remove_network_interface_event_callback(dps_interface_event_handler); #endif /* OC_NETWORK_MONITOR */ }This addition would make the intent clearer and improve code readability.
port/unittest/connectivitytest.cpp (1)
Line range hint
160-180
: LGTM! Consider adding more comprehensive tests.The addition of the
interface_event_handler
function and the modification of thehandle_network_interface_event_callback
test case look good. The changes improve the testability of the network monitoring feature.Consider expanding the test case to cover more scenarios, such as:
- Testing with different event types (not just NETWORK_INTERFACE_UP).
- Verifying the behavior when multiple callbacks are registered.
- Testing the order of callback execution if applicable.
This would provide more comprehensive coverage of the network monitoring functionality.
port/linux/ipadapter.c (2)
43-46
: LGTM! Consider moving the include statement.The conditional inclusion of
oc_network_monitor.h
is correct and allows for flexible compilation based on theOC_NETWORK_MONITOR
feature flag.For better code organization, consider moving this include statement to the block of other conditional includes (e.g., near the
OC_SESSION_EVENTS
andOC_TCP
blocks) to keep all feature-dependent includes together.
Line range hint
1-1330
: Consider refactoring for improved maintainability and readability.While the changes made are correct, there are several areas where the overall file could be improved:
File size: This file is quite long and handles multiple responsibilities. Consider splitting it into smaller, more focused files (e.g., separate files for IPv4 and IPv6 handling, event management, etc.).
Function complexity: Some functions, like
network_event_thread
,process_interface_change_event
, andinitialize_ip_context
, are quite long and complex. Consider refactoring these into smaller, more focused functions to improve readability and maintainability.Code duplication: There are instances of similar code for IPv4 and IPv6 handling. Consider creating shared utility functions to reduce duplication.
Error handling: Some error messages could be more descriptive. Consider adding more context to error messages, especially in network initialization and event handling sections.
Here's an example of how you might start refactoring the
initialize_ip_context
function:static bool initialize_ip_context_common(ip_context_t *dev, size_t device) { dev->device = device; OC_LIST_STRUCT_INIT(dev, eps); if (pthread_mutex_init(&dev->rfds_mutex, NULL) != 0) { oc_abort("error initializing TCP adapter mutex"); } if (pipe(dev->wakeup_pipe) < 0) { OC_ERR("wakeup pipe: %d", errno); return false; } if (!oc_fcntl_set_nonblocking(dev->wakeup_pipe[0])) { OC_ERR("Could not set non-block wakeup_pipe[0]"); return false; } return true; } static bool initialize_ip_context(ip_context_t *dev, size_t device, oc_connectivity_ports_t ports) { if (!initialize_ip_context_common(dev, device)) { return false; } if (!initialize_ip_context_ipv6(dev, ports)) { return false; } #ifdef OC_IPV4 if (!initialize_ip_context_ipv4(dev, ports)) { OC_ERR("Could not initialize IPv4"); return false; } #endif // ... rest of the function ... }This refactoring separates the common initialization logic and makes the function easier to read and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
- api/cloud/oc_cloud.c (1 hunks)
- api/oc_buffer_settings.c (1 hunks)
- api/oc_session_events.c (0 hunks)
- api/plgd/device-provisioning-client/plgd_dps.c (2 hunks)
- api/plgd/device-provisioning-client/plgd_dps_manager.c (0 hunks)
- api/plgd/unittest/plgdtimetest.cpp (0 hunks)
- api/unittest/buffersettingstest.cpp (1 hunks)
- include/oc_network_monitor.h (2 hunks)
- include/oc_session_events.h (2 hunks)
- include/oc_sp.h (2 hunks)
- port/android/ipadapter.c (1 hunks)
- port/esp32/adapter/src/ipadapter.c (1 hunks)
- port/linux/ipadapter.c (1 hunks)
- port/oc_poll_loop.h (2 hunks)
- port/oc_storage.h (2 hunks)
- port/unittest/connectivitytest.cpp (1 hunks)
- port/windows/ipadapter.c (1 hunks)
- swig/swig_interfaces/oc_session_events.i (1 hunks)
💤 Files not reviewed due to no reviewable changes (3)
- api/oc_session_events.c
- api/plgd/device-provisioning-client/plgd_dps_manager.c
- api/plgd/unittest/plgdtimetest.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- api/oc_buffer_settings.c
🔇 Additional comments (25)
include/oc_network_monitor.h (1)
38-38
: LGTM: OC_API annotation added correctlyThe addition of the
OC_API
annotation to theoc_add_network_interface_event_callback
function is correct and aligns with the PR objective. This clearly marks the function as part of the public API, which is a good practice for API design and documentation.port/oc_storage.h (4)
24-26
: LGTM: New includes added for API annotationsThe addition of "oc_export.h" and "util/oc_compiler.h" is appropriate for supporting the new OC_API and OC_NONNULL annotations. This change enhances consistency across the project and prepares the functions for use in the public API.
41-42
: LGTM: OC_API annotation added to oc_storage_configThe addition of the OC_API annotation to oc_storage_config is appropriate and aligns with the PR objectives. This change clearly marks the function as part of the public API, improving the interface's clarity for users of the library.
58-66
: LGTM: OC_API and OC_NONNULL added to oc_storage_writeThe modifications to oc_storage_write are well-implemented and consistent with the changes made to oc_storage_read:
- The OC_API annotation correctly marks this function as part of the public API.
- The OC_NONNULL() attribute enhances safety by enforcing non-null parameters.
- The updated comments clearly indicate which parameters cannot be NULL.
These changes improve both the usability and safety of the function, maintaining consistency across the API.
Line range hint
1-73
: Overall assessment: Excellent improvements to API clarity and safetyThe changes made to this file consistently enhance the public API by:
- Adding OC_API annotations to clearly mark public functions.
- Introducing OC_NONNULL attributes to improve function call safety.
- Updating comments to provide clearer documentation on parameter constraints.
These modifications align well with the PR objectives and significantly improve the usability and safety of the storage-related functions. The consistent application of these changes across all affected functions is commendable.
port/oc_poll_loop.h (8)
32-32
: LGTM: Necessary include for OC_API macroThe addition of
#include "oc_export.h"
is appropriate and necessary for theOC_API
macro used in the new function declarations.
42-43
: LGTM: Well-defined initialization functionThe
oc_poll_loop_init
function declaration is clear and consistent with the PR objectives. TheOC_API
annotation and the brief comment provide good context for its usage.
46-47
: LGTM: Clear shutdown function declarationThe
oc_poll_loop_shutdown
function declaration is appropriate and aligns with the PR objectives. TheOC_API
annotation and the brief comment provide good context for its usage.
50-51
: LGTM: Well-defined main loop execution functionThe
oc_poll_loop_run
function declaration is clear and consistent with the PR objectives. TheOC_API
annotation and the brief comment provide good context for its usage.
54-55
: LGTM: Clear signaling function for the main loopThe
oc_poll_loop_signal
function declaration is appropriate and aligns with the PR objectives. TheOC_API
annotation and the brief comment provide good context for its usage.
58-59
: LGTM: Well-defined termination function for the main loopThe
oc_poll_loop_terminate
function declaration is clear and consistent with the PR objectives. TheOC_API
annotation and the brief comment provide good context for its usage.
62-63
: LGTM: Clear status check function for main loop terminationThe
oc_poll_loop_is_terminated
function declaration is appropriate and aligns with the PR objectives. TheOC_API
annotation and the brief comment provide good context for its usage.
Line range hint
32-63
: Overall: Excellent addition of polling loop APIThe changes to this file are well-structured and align perfectly with the PR objectives. The new function declarations provide a comprehensive API for managing the polling loop, including initialization, execution, signaling, termination, and status checking. The consistent use of
OC_API
annotations and theOC_HAS_FEATURE_SIMPLE_MAIN_LOOP
feature flag demonstrate good coding practices.These additions enhance the functionality of the polling loop by providing clear mechanisms for its lifecycle management. The API is well-documented with brief comments, making it easier for developers to understand and use these functions.
api/unittest/buffersettingstest.cpp (1)
1-23
: LGTM: Proper file header and includes.The file starts with an appropriate Apache 2.0 license header and includes the necessary headers for the unit tests.
include/oc_session_events.h (3)
30-30
: LGTM: Utility header inclusionThe addition of the "util/oc_compiler.h" header is appropriate. It likely provides necessary compiler-specific macros or utilities for the new functionality introduced in this file.
105-115
: LGTM: Removal function with proper deprecation noticeThe addition of
oc_remove_session_event_callback
with a clear deprecation notice is well-implemented. The function signature, documentation, and return values are clearly defined.
Line range hint
1-133
: Overall assessment: Well-implemented API evolutionThe changes in this file represent a well-executed evolution of the session events API. The introduction of
v1
functions with additionaluser_data
parameters provides more flexibility for users while maintaining backward compatibility through properly deprecated older functions. The consistency between the newadd
andremove
functions is commendable.Key improvements:
- Added flexibility with
user_data
parameter in callback functions.- Proper deprecation of older functions.
- Consistent API design between new functions.
- Clear documentation of function signatures and return values.
These changes should make the API more versatile and easier to use in various contexts. The minor suggestions for documentation improvements, if implemented, will further enhance the usability of these new functions.
include/oc_sp.h (3)
28-29
: LGTM: Include directive for API export definitions.The addition of the
"oc_export.h"
include is appropriate. This header likely contains definitions for theOC_API
macro used later in the file, which is consistent with the PR objective of adding OC_API annotations.
131-133
: LGTM: OC_API annotation added to function declaration.The
OC_API
macro has been correctly added to theoc_pki_set_security_profile
function declaration. This change aligns with the PR objective of adding OC_API annotations to public API functions. The function signature remains unchanged, which preserves backward compatibility.
Line range hint
1-141
: Overall assessment: Changes are appropriate and align with PR objectives.The modifications to this file, including the addition of the
"oc_export.h"
include and theOC_API
annotation to theoc_pki_set_security_profile
function, are consistent with the PR objective of adding OC_API annotations. These changes improve the API visibility without introducing breaking changes. The implementation is clean and maintains the existing functionality.api/plgd/device-provisioning-client/plgd_dps.c (2)
33-36
: LGTM: Conditional inclusion of network monitoring headerThe addition of the conditional compilation directive for including "oc_network_monitor.h" is a good practice. It allows for more flexible compilation based on whether the OC_NETWORK_MONITOR feature is enabled.
Line range hint
1-542
: Overall assessment: Changes look goodThe modifications to conditionally include network monitoring functionality are well-implemented and consistent throughout the file. These changes improve the flexibility of the code by allowing selective compilation of network monitoring features based on the OC_NETWORK_MONITOR macro.
A minor suggestion was made to improve code readability by adding comments to explain empty function bodies when network monitoring is disabled.
api/cloud/oc_cloud.c (1)
43-45
: Approve: Good use of conditional compilation for optional featureThe introduction of conditional inclusion for
oc_network_monitor.h
is a positive change. It allows for more flexible compilation by including the network monitor functionality only when theOC_NETWORK_MONITOR
macro is defined. This approach is beneficial for:
- Reducing compilation time and binary size when the feature is not needed.
- Allowing easier configuration of builds with or without network monitoring.
- Improving code modularity by clearly separating optional features.
port/android/ipadapter.c (1)
43-46
: Approved: Good use of conditional compilation for optional feature.The addition of the conditional inclusion for "oc_network_monitor.h" is a good practice. It allows for the network monitoring functionality to be optionally compiled based on the OC_NETWORK_MONITOR macro. This approach helps in managing optional features and keeping the codebase modular.
port/esp32/adapter/src/ipadapter.c (1)
35-38
: Appropriate use of conditional compilationThe addition of the conditional include for "oc_network_monitor.h" is well-placed and follows good practices for optional feature inclusion. This change allows for more flexible compilation based on the presence of the network monitoring feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
port/oc_dns.h (2)
19-21
: Enhance the file header commentWhile the file header comment is present, it could be more informative. Consider adding a brief description of the file's purpose and the functionality it provides.
Here's a suggested improvement:
/** * @file + * @brief DNS lookup functionality for the OC (Open Connectivity) framework + * + * This file declares functions for DNS resolution and cache management. */
35-46
: Enhance documentation for oc_dns_lookup functionWhile the function documentation provides basic information, it could be more detailed to improve clarity and usability.
Consider expanding the documentation as follows:
/** * @brief dns look up * - * @param domain the url - * @param addr the address - * @param flags the transport flags - * @return int 0 = success + * @param domain The domain name to be resolved + * @param addr Pointer to oc_string_t where the resolved address will be stored + * @param flags Transport flags to specify the type of address resolution needed + * @return int 0 on successful resolution, non-zero value on failure + * + * This function performs a DNS lookup for the given domain name and stores + * the resolved address in the provided oc_string_t structure. The transport + * flags parameter can be used to specify additional requirements for the + * address resolution. */port/oc_connectivity_internal.h (1)
85-92
: LGTM: Updated documentation foroc_connectivity_wakeup
is clear.The expanded documentation for
oc_connectivity_wakeup
provides better clarity on the function's purpose. The#ifdef OC_DYNAMIC_ALLOCATION
block appropriately limits the function's availability to when dynamic allocation is enabled.Consider adding a brief note about why this function is only available with dynamic allocation, if there's a specific reason. This could help developers understand the constraints and use cases better.
port/oc_connectivity.h (3)
152-154
: LGTM: API visibility and deprecation notice addedThe addition of
OC_API
andOC_DEPRECATED
macros tooc_connectivity_end_session
is appropriate. It clearly indicates that the function is part of the public API but is deprecated.Consider the following suggestions:
- Update the function's documentation to prominently mention its deprecated status.
- Add a link or reference to the documentation for the new
oc_close_session
function.- If not already done, ensure that release notes or migration guides mention this deprecation.
162-163
: LGTM: API visibility addedThe addition of the
OC_API
macro tooc_connectivity_get_endpoints
is appropriate, clearly indicating that this function is part of the public API.Consider enhancing the function's documentation:
- Add information about the lifecycle of the returned
oc_endpoint_t*
(e.g., whether the caller is responsible for freeing it).- Specify the behavior when no endpoints are available (e.g., returns NULL or an empty list).
- If applicable, mention any thread-safety considerations for this function.
Line range hint
1-280
: Overall assessment: Positive changes improving API clarity and organizationThe modifications in this file contribute to better API management and code organization:
- DNS functionality has been modularized by moving it to a separate header file.
- Public API functions are now clearly marked with the
OC_API
macro.- Deprecated functions are properly annotated, guiding users towards newer alternatives.
These changes should improve the maintainability of the codebase and provide clearer guidance to API users. The consistent application of these changes suggests a systematic approach to API management.
As the project continues to evolve:
- Consider implementing a formal API versioning system if not already in place.
- Regularly review and update API documentation to reflect these changes.
- Maintain a comprehensive changelog or migration guide to assist users in adapting to API changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- api/unittest/buffersettingstest.cpp (1 hunks)
- port/oc_connectivity.h (2 hunks)
- port/oc_connectivity_internal.h (2 hunks)
- port/oc_dns.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/unittest/buffersettingstest.cpp
🔇 Additional comments (7)
port/oc_dns.h (3)
23-33
: LGTM: Header guards and includes are well-structuredThe header guards, necessary includes, and C++ compatibility handling are all correctly implemented.
Also applies to: 60-64
1-64
: Overall, the new DNS functionality is well-structuredThe introduction of DNS lookup functionality in this header file is well-organized with proper header guards, includes, and conditional compilation. The use of the OC_API macro for function declarations is consistent and appropriate.
Key points:
- The file structure follows good practices for C header files.
- Conditional compilation allows for flexible feature inclusion.
- Function declarations are clear and use appropriate types.
Areas for improvement:
- Enhance documentation for both functions to provide more context and usage details.
- Verify the implementation of
oc_dns_clear_cache
in the corresponding source file.These improvements will enhance the usability and maintainability of the DNS functionality in the OC framework.
48-56
: Enhance documentation for oc_dns_clear_cache function and clarify implementationThe function documentation for
oc_dns_clear_cache
is minimal and could be expanded to provide more context.Consider enhancing the documentation as follows:
/** * @brief clear the DNS cache + * + * This function clears the DNS cache, removing all stored DNS resolutions. + * It's useful for ensuring fresh DNS lookups or managing memory usage. + * + * @note This function is only available when OC_DNS_CACHE is defined. */Additionally, the implementation details of this function are not provided in the header file.
Could you clarify where the implementation of this function can be found? It would be helpful to ensure that the implementation exists and matches the intended functionality.
port/oc_connectivity_internal.h (3)
56-63
: LGTM: New functionoc_send_buffer
looks good.The new function
oc_send_buffer
is well-documented and consistent with the existing code style. The function signature and return value definitions are clear and appropriate for the task.
78-83
: LGTM: New functionoc_send_discovery_request
is appropriately defined.The new function
oc_send_discovery_request
is consistent with the existing code style and has an appropriate signature for its purpose. The documentation, while minimal, is sufficient for an internal header file.
Line range hint
56-92
: Summary: Enhancements to connectivity API look good overall.The changes to
port/oc_connectivity_internal.h
introduce two new functions (oc_send_buffer
andoc_send_discovery_request
) and improve the documentation foroc_connectivity_wakeup
. These additions enhance the connectivity API's functionality and clarity. The changes are consistent with the existing code style and conventions, and there are no apparent issues with the implementations.port/oc_connectivity.h (1)
32-32
: LGTM: DNS functionality modularizationThe addition of
#include "port/oc_dns.h"
is a good step towards better code organization. It aligns with the removal of DNS-related functions from this file.To ensure completeness, please verify that all necessary DNS-related declarations are present in the new
oc_dns.h
file:✅ Verification successful
Verification Successful: DNS Declarations Present
The
port/oc_dns.h
file exists and contains the necessary DNS-related function declarations. The inclusion inport/oc_connectivity.h
is correctly handled.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the contents of the new oc_dns.h file # Expected: File should exist and contain DNS-related function declarations cat port/oc_dns.hLength of output: 1468
a9fb620
to
fae64b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
port/oc_connectivity_internal.h (1)
116-124
: Approved: Deprecated functionoc_connectivity_end_session
is clearly marked.The deprecation notice and use of the OC_DEPRECATED macro are good practices. This helps developers transition to the new
oc_close_session
function.Consider adding a TODO comment to remove this function in a future version, helping to track technical debt:
+// TODO: Remove this function in a future version (e.g., v2.3.0) void oc_connectivity_end_session(const oc_endpoint_t *endpoint) OC_DEPRECATED("replaced by oc_close_session in v2.2.5.14");
security/oc_oscore_engine.c (1)
Add OSCORE Functionality Tests
It appears that there are no existing OSCORE-related test files. To ensure that the recent inclusion of the new include statement does not introduce any unintended side effects, please add comprehensive tests covering OSCORE encryption, decryption, and message handling processes.
🔗 Analysis chain
Line range hint
1-1024
: Ensure thorough testing of OSCORE functionality.While the only visible change is the addition of a new include statement, it's important to verify that this change doesn't introduce any unintended side effects. Please ensure that all OSCORE-related functionality is thoroughly tested, including encryption, decryption, and message handling processes.
To help verify the OSCORE functionality, you can run the following commands:
This will help identify the existing OSCORE tests and their contents, allowing you to ensure that all relevant scenarios are covered.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for OSCORE-related test files and their contents # Find OSCORE-related test files echo "OSCORE-related test files:" fd -e c -e cpp -e h oscore tests # Check contents of OSCORE-related test files echo "\nContents of OSCORE-related test files:" fd -e c -e cpp -e h oscore tests -x catLength of output: 230
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- api/oc_message_buffer.c (1 hunks)
- api/unittest/buffersettingstest.cpp (1 hunks)
- port/oc_connectivity.h (2 hunks)
- port/oc_connectivity_internal.h (4 hunks)
- port/oc_dns.h (1 hunks)
- security/oc_oscore_engine.c (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/unittest/buffersettingstest.cpp
- port/oc_connectivity.h
- port/oc_dns.h
🔇 Additional comments (3)
port/oc_connectivity_internal.h (2)
57-64
: LGTM: New functionoc_send_buffer
looks good.The function signature, documentation, and return value definitions are clear and consistent with the file's style. The addition of this function enhances the connectivity functionality as intended.
78-83
: LGTM: New functionoc_send_discovery_request
is well-defined.The function signature and documentation are clear and consistent with the file's style. This addition enhances the discovery functionality within the connectivity module.
api/oc_message_buffer.c (1)
Line range hint
1-214
: Ensure thorough testing of network-related functionalities.Given the change in the connectivity header and the presence of network-related functions in this file (e.g.,
oc_send_buffer
,oc_send_discovery_request
), it's crucial to verify that all network operations continue to function correctly.Please ensure that:
- All network-related functions used in this file are still accessible and behave as expected.
- The change doesn't introduce any unintended side effects in the network communication logic.
Consider running comprehensive integration tests that cover:
- Inbound and outbound message handling
- Discovery requests
- Unicast and multicast communication
- Secure communication (if applicable)
This will help catch any potential issues introduced by the header change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
include/oc_signal_event_loop.h (2)
33-34
: LGTM: OC_API macro added correctly.The addition of the
OC_API
macro to the_oc_signal_event_loop
function declaration is correct and aligns with the PR objectives. This change will likely affect the function's visibility and linkage in other translation units.Consider renaming the function to remove the leading underscore, as identifiers beginning with an underscore followed by a capital letter or another underscore are reserved for use by the compiler and standard library. A suggested name could be
oc_signal_event_loop
.
24-34
: Summary: OC_API annotations successfully added.The changes in this file successfully add the OC_API annotations as per the PR objectives. The addition of the
oc_export.h
include and theOC_API
macro to the function declaration will likely improve API visibility and standardization across the codebase. These changes appear to be part of a larger effort to refactor and improve the project's API.As you continue this refactoring effort, consider reviewing other header files for consistency in applying the OC_API macro to public functions. This will ensure a uniform API across the project.
api/oc_rep_internal.h (2)
86-93
: LGTM! Consider adding a note about thread safety.The function declaration and its documentation look good. The return type and lack of parameters are consistent with the function's purpose of retrieving the start of the CBOR encoded buffer.
Consider adding a note about thread safety, especially if this function accesses shared state. For example:
* @return pointer to the start of the cbor encoded buffer * * @see oc_parse_rep + * @note This function is not thread-safe and should only be called from the main thread. */
95-103
: LGTM! Consider clarifying ownership and adding const qualifier.The function declaration and its documentation are generally good. However, there are a few points that could be improved for clarity and safety:
- Clarify ownership in the documentation:
* Shrink the buffer pointer to length of encoded cbor data. * * @param[in] buf pointer to cbor encoded buffer * @return pointer to the start of the shrinked cbor encoded buffer + * @note The returned pointer may or may not be the same as the input pointer. + * The caller should not free the returned pointer separately. * * @see oc_parse_rep + * @note This function is not thread-safe and should only be called from the main thread. */
- Consider adding the
const
qualifier to the input parameter:-uint8_t *oc_rep_shrink_encoder_buf(uint8_t *buf); +uint8_t *oc_rep_shrink_encoder_buf(const uint8_t *buf);This change would prevent accidental modification of the input buffer and make the function's intent clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- api/oc_mnt.c (1 hunks)
- api/oc_rep_internal.h (1 hunks)
- api/unittest/helperstest.cpp (1 hunks)
- include/oc_rep.h (12 hunks)
- include/oc_signal_event_loop.h (1 hunks)
- messaging/coap/observe.c (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/unittest/helperstest.cpp
🔇 Additional comments (4)
include/oc_signal_event_loop.h (1)
24-25
: LGTM: Appropriate include added for OC_API macro.The addition of the
oc_export.h
include is consistent with the use of theOC_API
macro later in the file. Good job on placing it in the correct location within the file structure.api/oc_mnt.c (1)
25-25
: Approve the addition of the include directive, but clarification needed.The addition of
#include "api/oc_rep_internal.h"
is logically placed and consistent with the AI-generated summary. However, I have a few points for clarification:
Could you please explain the necessity of including this header? The existing code doesn't seem to use any new functions from it, so it might be helpful to understand if this is preparation for future changes or resolves some implicit dependencies.
The PR title mentions adding OC_API annotations, but this change doesn't directly relate to that. Could you clarify how this include directive contributes to the PR's main objective?
To ensure this include is necessary and not redundant, let's check if oc_rep_internal.h is already indirectly included:
✅ Verification successful
Include directive addition verified.
The
#include "api/oc_rep_internal.h"
is not included indirectly elsewhere inapi/oc_mnt.c
, making the explicit addition necessary to ensure the required internal representations are available.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if oc_rep_internal.h is already included indirectly for header in $(grep -oP '#include "\K[^"]+' api/oc_mnt.c | grep -v 'api/oc_rep_internal.h'); do if grep -q '#include "api/oc_rep_internal.h"' "$header" 2>/dev/null; then echo "oc_rep_internal.h is already included in $header" exit 0 fi done echo "oc_rep_internal.h is not included indirectly"Length of output: 1775
messaging/coap/observe.c (1)
58-58
: Verify the necessity of the new include statement.The addition of
#include "api/oc_rep_internal.h"
suggests that functionality from this header is now required. However, there are no visible changes in the rest of the file that directly use this new include.Could you please verify if this include is necessary for any indirect usage or upcoming changes? If it's not being used, consider removing it to avoid potential confusion and maintain clean dependencies.
To check for any usage of symbols from this header, you can run the following command:
If this command doesn't return any results, it might indicate that the include is not necessary.
✅ Verification successful
The new include statement is necessary.
The addition of
#include "api/oc_rep_internal.h"
is required as multiple functions prefixed withoc_rep_
are utilized withinmessaging/coap/observe.c
. This ensures that the necessary symbols and functionalities are correctly referenced.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for usage of symbols from oc_rep_internal.h grep -rn "oc_rep_" messaging/coap/observe.cLength of output: 1666
include/oc_rep.h (1)
132-132
: Addition ofOC_API
andOC_NONNULL()
Annotations Enhances API SafetyThe inclusion of
OC_API
andOC_NONNULL()
annotations in the function declarations improves the clarity of the public API and enforces non-null constraints on pointers, enhancing code safety and usability.Also applies to: 141-141, 151-151, 161-161, 171-171, 184-184, 194-194, 206-206
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (11)
include/oc_ri.h (3)
Line range hint
637-652
: LGTM: API updates, improved documentation, and deprecation notice.The changes include:
- Addition of OC_API annotations to
oc_ri_get_query_value
andoc_ri_query_exists_v1
functions.- Updated comments using doxygen-style tags, improving documentation clarity.
- Addition of OC_NONNULL and OC_DEPRECATED macros to
oc_ri_get_query_value
.- Introduction of
oc_ri_query_exists_v1
, likely replacing the deprecatedoc_ri_query_exists
.These changes improve the API's documentation and signal important updates to consumers. The deprecation notice is particularly important.
Consider adding a comment to
oc_ri_query_exists_v1
directing users to use this function instead of the deprecatedoc_ri_query_exists
. This will help guide API consumers towards the new function.
Line range hint
663-680
: LGTM: API updates, improved documentation, and deprecation notice.The changes include:
- Addition of OC_API annotations to
oc_ri_query_exists
andoc_ri_query_nth_key_exists
functions.- Updated comments using doxygen-style tags, improving documentation clarity.
- Addition of OC_NONNULL and OC_DEPRECATED macros to
oc_ri_query_exists
.These changes improve the API's documentation and signal important updates to consumers. The deprecation notice for
oc_ri_query_exists
is particularly important.If
oc_ri_query_nth_key_exists
is intended to replaceoc_ri_query_exists
, consider adding a comment tooc_ri_query_nth_key_exists
directing users to use this function instead of the deprecated one. This will help guide API consumers towards the new function.
Line range hint
1-689
: Summary: Improved API documentation and consistencyOverall, the changes to
include/oc_ri.h
significantly improve the file's quality and usability:
- Consistent comment style throughout the file, improving readability.
- Addition of OC_API annotations, clearly marking public API functions.
- Updated function documentation using doxygen-style tags, enhancing clarity.
- Proper marking of deprecated functions and introduction of their replacements.
- Addition of OC_NONNULL macros, improving function safety.
These changes will benefit users of the library by providing clearer documentation and API guidelines. The deprecation notices and new function introductions suggest an evolving API, which users should be aware of when updating to this version.
As the API evolves, consider providing a migration guide or updating the main documentation to help users transition from deprecated functions to their new counterparts. This will ensure a smoother upgrade process for consumers of the library.
api/oc_ri_server_internal.h (5)
90-95
: Add missing documentation foroc_ri_handle_observation
functionThe function
oc_ri_handle_observation
lacks a detailed documentation comment block. For consistency and clarity, please add a documentation block that describes the purpose of the function, its parameters, and return value.
96-100
: Expand documentation foroc_ri_notify_resource_observers
functionThe documentation for
oc_ri_notify_resource_observers
is minimal. Consider adding more detailed comments, including descriptions of the parameters and any important notes about the function's behavior.
101-103
: Clarify the purpose ofoc_ri_server_init
functionWhile the function
oc_ri_server_init
is intended to initialize server variables, additional details about what specifically is being initialized would enhance readability and maintainability. Please consider expanding the documentation to include this information.
104-106
: Clarify the functionality ofoc_ri_server_reset
functionThe description "Reset observations" may not fully convey the scope of the function. Provide additional details in the documentation to explain what aspects are reset and any implications this may have on the server state.
107-109
: Add documentation foroc_ri_server_shutdown
functionThe function
oc_ri_server_shutdown
currently lacks a documentation comment. To maintain consistency and aid future developers, please add a comment block describing its purpose and any important considerations when calling it.api/oc_ri_server.c (3)
Line range hint
263-274
: Functionoc_ri_on_delete_resource_find_callback
used before its declarationIn the function
oc_ri_on_delete_resource_add_callback
, the call tooc_ri_on_delete_resource_find_callback(cb)
occurs before the functionoc_ri_on_delete_resource_find_callback
is declared or defined. This can lead to compilation errors or warnings about implicit function declarations.To fix this, ensure that the declaration or definition of
oc_ri_on_delete_resource_find_callback
precedes its usage inoc_ri_on_delete_resource_add_callback
. You can rearrange the code as follows:+// Move the definition of oc_ri_on_delete_resource_find_callback before its usage + oc_ri_on_delete_resource_t * oc_ri_on_delete_resource_find_callback(oc_ri_delete_resource_cb_t cb) { oc_ri_on_delete_resource_t *item = oc_list_head(g_on_delete_resource_cb_list); for (; item != NULL; item = item->next) { if (cb == item->cb) { return item; } } return NULL; } bool oc_ri_on_delete_resource_add_callback(oc_ri_delete_resource_cb_t cb) { if (oc_ri_on_delete_resource_find_callback(cb) != NULL) { OC_ERR("delete resource callback already exists"); return false; } // Rest of the function... }🧰 Tools
🪛 cppcheck
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
70-74
: Unused functionoc_ri_alloc_resource
The function
oc_ri_alloc_resource
is defined but not used within the provided code. While it's acceptable for utility functions to exist without immediate usage, ensure that this function is necessary and utilized appropriately in the codebase.If the function is intended for future use or is used elsewhere, you can disregard this comment. Otherwise, consider removing it to keep the code clean.
🧰 Tools
🪛 cppcheck
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
76-80
: Unused functionoc_ri_dealloc_resource
Similarly, the function
oc_ri_dealloc_resource
is defined but not used within the provided code. Verify if this function is required.If it's intended for memory management in other parts of the code, ensure it's being called where necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- api/oc_collection.c (1 hunks)
- api/oc_ri.c (5 hunks)
- api/oc_ri_internal.h (0 hunks)
- api/oc_ri_server.c (2 hunks)
- api/oc_ri_server_internal.h (2 hunks)
- include/oc_ri.h (16 hunks)
💤 Files with no reviewable changes (1)
- api/oc_ri_internal.h
🧰 Additional context used
🪛 cppcheck
api/oc_ri_server.c
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (11)
include/oc_ri.h (6)
Line range hint
40-48
: LGTM: Improved comment style consistency.The change from multi-line to single-line comments for the CoAP methods enumeration improves the overall consistency of the code. The content remains the same, ensuring no loss of information.
Line range hint
49-68
: LGTM: Improved comment style and added informative note.The change to single-line comments for resource properties and response status enumerations improves code consistency. The added note about status code translation to HTTP or CoAP is informative and helpful for developers.
Line range hint
160-176
: LGTM: Consistent comment style for interface masks.The change to single-line comments for the interface masks enumeration improves the overall consistency of the code. The content remains unchanged, maintaining the same level of information.
Line range hint
227-294
: LGTM: Consistent comment style for structures and typedefs.The change to single-line comments for various structures and typedefs improves the overall consistency of the code. The content remains essentially the same, with some minor rewording that doesn't affect the meaning. This change enhances readability without altering functionality.
Line range hint
610-627
: LGTM: Improved API annotations, documentation, and safety checks.The changes to
oc_ri_get_query_nth_key_value
andoc_ri_get_query_value_v1
functions include:
- Addition of OC_API annotations, indicating they are part of the public API.
- Updated comments using doxygen-style @param and @return tags, improving documentation clarity.
- Addition of OC_NONNULL macro, likely enforcing that certain parameters cannot be NULL, which improves function safety.
These changes enhance the API's usability, documentation, and robustness without altering the core functionality.
Line range hint
507-516
: LGTM: Added OC_API annotations to public functions.The addition of OC_API annotations to
oc_ri_get_app_resource_by_uri
andoc_ri_get_app_resources
functions indicates that these are part of the public API. This change may affect how these functions are exported or used by consumers of the library, but it doesn't alter the functionality of the code itself.To ensure this change doesn't introduce any inconsistencies, let's check if other similar functions in the file also have the OC_API annotation:
✅ Verification successful
Verified: OC_API annotations are consistently applied to public functions.
The addition of
OC_API
annotations tooc_ri_get_app_resource_by_uri
andoc_ri_get_app_resources
aligns with existing annotations in the file, ensuring these functions are correctly exposed as part of the public API.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other function declarations in the file grep -n "OC_API" include/oc_ri.hLength of output: 285
api/oc_collection.c (3)
28-28
: LGTM: Include statement added for internal server functionality.The addition of
#include "api/oc_ri_server_internal.h"
is appropriate as it likely provides access to internal server-related functions and definitions that may be needed for collection handling. This change appears to be consistent with the file's purpose and existing includes.
Line range hint
1-1009
: Summary: Minor change with potential for future improvements.The addition of the include statement for "oc_ri_server_internal.h" is a minor change that doesn't introduce any immediate issues. The file's overall structure and functionality remain intact. However, this new include may provide opportunities for future code improvements or optimizations using the newly available internal server functions. Consider reviewing the usage of this header in the context of the entire file in a future refactoring task.
Line range hint
1-1009
: Consider reviewing usage of newly included header.While the addition of the new include statement is appropriate, it may be beneficial to review the contents of "oc_ri_server_internal.h" and consider if any of its newly available functions or definitions could be used to improve or simplify the existing code in this file.
To ensure that the new include is being utilized effectively, you can run the following command to check for any usage of symbols from the newly included header:
✅ Verification successful
Verify Necessity of "oc_ri_server_internal.h" Inclusion
The inclusion of
"oc_ri_server_internal.h"
inapi/oc_collection.c
is currently utilized by only one function:oc_ri_URI_is_in_use
. If this function is essential and there's no alternative in a more commonly included header, retaining the inclusion is justified. However, if possible, consider refactoring to minimize dependencies by relocating the function or using forward declarations where appropriate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of symbols from oc_ri_server_internal.h # Get all function and type names from oc_ri_server_internal.h header_symbols=$(rg --no-filename '(typedef|struct|enum|union|#define|\w+\s+\w+\s*\()' api/oc_ri_server_internal.h | awk '{print $2}' | cut -d'(' -f1 | sort -u) # Check for usage of these symbols in the current file for symbol in $header_symbols; do rg "\b$symbol\b" api/oc_collection.c doneLength of output: 1750
api/oc_ri_server_internal.h (1)
44-45
: Ensure consistent use ofOC_NONNULL
macroThe function
oc_ri_URI_is_in_use
is annotated withOC_NONNULL()
, indicating that certain parameters must not beNULL
. Verify that all functions with non-nullable pointer parameters are consistently annotated withOC_NONNULL()
to enforce this contract throughout the codebase.api/oc_ri.c (1)
1304-1307
: Validate Parameters foroc_ri_handle_observation()
The updated call to
oc_ri_handle_observation()
in GET requests correctly handles observation options. However, ensure that all parameters passed to the function are valid and that any potential errors are handled properly.Run the following script to verify the correctness of parameters used in all calls to
oc_ri_handle_observation()
:This script searches for all occurrences of
oc_ri_handle_observation()
in the codebase and displays surrounding lines for context. Review these instances to ensure parameters are correctly used.
86e4940
to
72c27a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
api/oc_ri_server.c (7)
93-130
: LGTM: Comprehensive resource addition functionThe
oc_ri_add_resource
function is well-implemented with thorough error checking and validation. It ensures that resources are not duplicated and have valid handlers.Consider grouping related checks together for better readability. For example, move the URI check closer to the other resource property checks:
if ((resource->properties & OC_PERIODIC) != 0 && resource->observe_period_seconds == 0) { OC_ERR("resource(%s) has invalid observe period", oc_string(resource->uri)); return false; } +if (oc_ri_URI_is_in_use(resource->device, oc_string(resource->uri), + oc_string_len(resource->uri))) { + OC_ERR("resource(%s) URI is already in use", oc_string(resource->uri)); + return false; +} if (oc_ri_is_app_resource_valid(resource)) { OC_ERR("resource(%s) already exists in IoTivity stack", oc_string(resource->uri)); return false; } if (oc_ri_is_app_resource_to_be_deleted(resource)) { OC_ERR("resource(%s) is scheduled to be deleted", oc_string(resource->uri)); return false; } -if (oc_ri_URI_is_in_use(resource->device, oc_string(resource->uri), - oc_string_len(resource->uri))) { - OC_ERR("resource(%s) URI is already in use", oc_string(resource->uri)); - return false; -}
132-166
: LGTM: Efficient resource retrieval functionsThe
oc_ri_get_app_resources
andoc_ri_get_app_resource_by_uri
functions are well-implemented and provide necessary functionality for resource management.Consider optimizing
oc_ri_get_app_resource_by_uri
for cases where the URI starts with a slash:oc_resource_t * oc_ri_get_app_resource_by_uri(const char *uri, size_t uri_len, size_t device) { if (!uri || uri_len == 0) { return NULL; } - int skip = 0; - if (uri[0] != '/') { - skip = 1; - } + const char *compare_uri = uri; + size_t compare_len = uri_len; + if (uri[0] == '/') { + compare_uri++; + compare_len--; + } oc_resource_t *res = oc_ri_get_app_resources(); while (res != NULL) { - if (oc_string_len(res->uri) == (uri_len + skip) && - strncmp(uri, oc_string(res->uri) + skip, uri_len) == 0 && + if (oc_string_len(res->uri) - 1 == compare_len && + strncmp(compare_uri, oc_string(res->uri) + 1, compare_len) == 0 && res->device == device) { return res; } res = res->next; } // ... rest of the function }This change eliminates the need for the
skip
variable and simplifies the comparison logic.
168-231
: LGTM: Comprehensive resource and URI validation functionsThe new functions for resource validation and URI checking are well-implemented and cover all necessary cases, including core resources, dynamic resources, and collections.
Consider optimizing the URI comparison in
ri_uri_is_in_list
to avoid repeated string manipulations:static bool ri_uri_is_in_list(oc_list_t list, const char *uri, size_t uri_len, size_t device) { + const char *compare_uri = uri; + size_t compare_len = uri_len; while (uri[0] == '/') { - uri++; - uri_len--; + compare_uri++; + compare_len--; } const oc_resource_t *res = oc_list_head(list); for (; res != NULL; res = res->next) { - if (res->device == device && oc_string_len(res->uri) == (uri_len + 1) && - strncmp(oc_string(res->uri) + 1, uri, uri_len) == 0) { + if (res->device == device && oc_string_len(res->uri) == (compare_len + 1) && + strncmp(oc_string(res->uri) + 1, compare_uri, compare_len) == 0) { return true; } } return false; }This change reduces the number of string manipulations performed in the loop, potentially improving performance for long lists of resources.
Line range hint
233-415
: LGTM: Comprehensive resource deletion functionalityThe resource deletion functions, including the delayed deletion mechanism, are well-implemented and cover all necessary aspects of resource cleanup.
Consider adding error handling in the
oc_delayed_delete_resource_cb
function:static oc_event_callback_retval_t oc_delayed_delete_resource_cb(void *data) { oc_resource_t *resource = (oc_resource_t *)data; OC_DBG("delayed delete resource(%p)", (void *)resource); oc_ri_on_delete_resource_invoke(resource); - oc_delete_resource(resource); + if (!oc_delete_resource(resource)) { + OC_ERR("Failed to delete resource(%p) in delayed callback", (void *)resource); + // Consider if any recovery action is needed here + } return OC_EVENT_DONE; }This change adds error checking for the
oc_delete_resource
call, which could fail if the resource has already been deleted or if there's an issue with the deletion process.
527-662
: LGTM: Enhanced observation handlingThe modifications to the observation handling functions improve support for collections and periodic observables. The code structure is clear and logical.
Consider enhancing error handling in the
ri_add_observation
function:static bool ri_add_observation(const coap_packet_t *request, const coap_packet_t *response, oc_resource_t *resource, bool resource_is_collection, uint16_t block2_size, const oc_endpoint_t *endpoint, oc_interface_mask_t iface_query) { - if (ri_observe_handler(request, response, resource, block2_size, endpoint, - iface_query) >= 0) { + int observe_result = ri_observe_handler(request, response, resource, block2_size, endpoint, + iface_query); + if (observe_result > 0) { + // Observation was unregistered + return true; + } else if (observe_result == 0) { /* If the resource is marked as periodic observable it means it must be * polled internally for updates (which would lead to notifications being * sent). If so, add the resource to a list of periodic GET callbacks to * utilize the framework's internal polling mechanism. */ if ((resource->properties & OC_PERIODIC) != 0 && !oc_periodic_observe_callback_add(resource)) { + OC_ERR("Failed to add periodic observe callback for resource(%p)", (void *)resource); return false; } + } else { + OC_ERR("Failed to handle observation for resource(%p)", (void *)resource); + return false; } #ifdef OC_COLLECTIONS if (resource_is_collection) { oc_collection_t *collection = (oc_collection_t *)resource; if (!ri_add_collection_observation(collection, endpoint, iface_query == OC_IF_B)) { - // TODO: shouldn't we remove the periodic observe callback here? + OC_ERR("Failed to add collection observation for resource(%p)", (void *)resource); + if ((resource->properties & OC_PERIODIC) != 0) { + oc_periodic_observe_callback_remove(resource); + } return false; } } #else /* !OC_COLLECTIONS */ (void)resource_is_collection; #endif /* OC_COLLECTIONS */ return true; }These changes provide more detailed error handling and logging, and address the TODO comment about removing the periodic observe callback when adding a collection observation fails.
664-693
: LGTM: ETag functionality for resources and batch operationsThe new ETag-related functions provide a good mechanism for handling ETags in different scenarios, including discovery and collections.
Consider modifying the default case in
oc_ri_get_batch_etag
to provide more information:uint64_t oc_ri_get_batch_etag(const oc_resource_t *resource, const oc_endpoint_t *endpoint, size_t device) { #ifdef OC_RES_BATCH_SUPPORT if (oc_core_get_resource_by_index(OCF_RES, device) == resource) { return oc_discovery_get_batch_etag(endpoint, device); } #endif /* OC_RES_BATCH_SUPPORT */ #ifdef OC_COLLECTIONS if (oc_check_if_collection(resource)) { return oc_collection_get_batch_etag((const oc_collection_t *)resource); } #endif /* OC_COLLECTIONS */ (void)resource; (void)endpoint; (void)device; - OC_DBG("custom batch etag"); + OC_DBG("Unhandled case in oc_ri_get_batch_etag for resource type: %d", resource->types); return OC_ETAG_UNINITIALIZED; }This change provides more context in the debug log when an unhandled case is encountered, which could be helpful for debugging and future extensions of the function.
Line range hint
1-694
: Final Review: Significant improvements with some areas for refinementThe changes to
api/oc_ri_server.c
significantly enhance the resource management capabilities of the IoT framework. Key improvements include:
- More robust resource allocation and deallocation.
- Enhanced observation handling, especially for collections and periodic observables.
- Addition of ETag support for resources and batch operations.
Overall, the code quality is good, with clear structure and logical organization. However, there are several areas where the code could be further improved:
- Error handling: While present, error handling could be more consistent and comprehensive throughout the file.
- Memory management: Consider adding checks for allocation failures in resource creation functions.
- Modularity: The file covers multiple concerns; consider splitting it into more focused modules.
- Documentation: Add more detailed function documentation, especially for complex functions.
- TODOs: Address or convert existing TODOs into tracked issues for future improvements.
To further improve the codebase:
- Implement a consistent error handling strategy, possibly using error code enums.
- Consider creating a resource context structure to encapsulate global state.
- Review the use of conditional compilation to ensure it doesn't overly complicate the code.
- Implement unit tests for the new and modified functions to ensure reliability.
These changes will enhance the maintainability, reliability, and performance of the resource management system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- api/cloud/unittest/cloud_store_test.cpp (1 hunks)
- api/oc_collection.c (1 hunks)
- api/oc_ri.c (5 hunks)
- api/oc_ri_internal.h (0 hunks)
- api/oc_ri_server.c (2 hunks)
- api/oc_ri_server_internal.h (2 hunks)
- include/oc_ri.h (16 hunks)
💤 Files with no reviewable changes (1)
- api/oc_ri_internal.h
🚧 Files skipped from review as they are similar to previous changes (3)
- api/oc_collection.c
- api/oc_ri_server_internal.h
- include/oc_ri.h
🧰 Additional context used
🪛 cppcheck
api/oc_ri_server.c
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (7)
api/cloud/unittest/cloud_store_test.cpp (1)
25-25
: LGTM. Verify usage of new include.The addition of
#include "api/oc_rep_internal.h"
looks good. However, please ensure that this header is actually used within this test file. If it's not directly used, consider removing it to avoid unnecessary includes.To verify the usage of the newly added include, you can run the following command:
If this command returns results, it indicates that the include is being used. If not, you may want to reconsider its inclusion.
✅ Verification successful
Verified inclusion of
api/oc_rep_internal.h
.The include is actively used in
cloud_store_test.cpp
, ensuring its necessity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if oc_rep_internal.h is used in the file # Test: Search for usage of functions or types from oc_rep_internal.h rg --type cpp -e 'oc_rep_' -e 'OC_REP_' api/cloud/unittest/cloud_store_test.cppLength of output: 2786
api/oc_ri_server.c (2)
20-56
: LGTM: New includes and structure definitionThe new includes and the
oc_resource_defaults_data_t
structure definition are well-organized and appropriate for the added functionality.
Line range hint
1-694
: Overall structure and documentation suggestionsThe file structure is generally well-organized, grouping related functions together and using conditional compilation for flexibility. However, there are a few areas that could be improved:
Consider adding more detailed function documentation, especially for complex functions like
ri_delete_resource
andri_add_observation
.There are some TODOs left in the code (e.g., line 516-518). It would be beneficial to address these or convert them into tracked issues if they represent longer-term improvements.
The file mixes different levels of abstraction (e.g., low-level memory management with high-level resource handling). Consider if some of these could be separated into different files for better modularity.
To improve the overall architecture and maintainability of the code:
Consider creating separate files for different concerns, such as:
- Resource allocation and deallocation
- Observation handling
- ETag management
Implement a more robust error handling and logging strategy throughout the file. This could include creating error code enums and using them consistently across functions.
Review the use of global variables (e.g.,
g_app_resources
) and consider if some of these could be encapsulated in a context structure for better state management.To ensure consistent error handling and logging, run the following script:
This script will help identify areas where error handling and logging can be improved for consistency.
api/oc_ri.c (4)
1304-1304
: LGTM: Improved observation handlingThe modification to
oc_ri_handle_observation
call with additional parameters enhances the context provided for observation handling. This change appears to be consistent with the updated function signature and improves the overall functionality.
327-327
: 🛠️ Refactor suggestionConsider adding error handling for
oc_ri_server_init()
The addition of
oc_ri_server_init();
is good for initializing server-specific components. However, it's recommended to check the return value and handle any potential errors during the initialization process.Consider applying this diff to add error handling:
#ifdef OC_SERVER - oc_ri_server_init(); + if (oc_ri_server_init() != 0) { + OC_ERR("Server initialization failed"); + return; + } #endif /* OC_SERVER */Likely invalid or redundant comment.
1386-1386
: 🛠️ Refactor suggestionConsider adding error handling for
oc_ri_server_shutdown()
The addition of
oc_ri_server_shutdown();
ensures proper cleanup of server-specific components. However, it's recommended to check the return value and handle any potential errors during the shutdown process.Consider applying this diff to add error handling:
#ifdef OC_SERVER - oc_ri_server_shutdown(); + if (oc_ri_server_shutdown() != 0) { + OC_ERR("Server shutdown failed"); + } #endif /* OC_SERVER */Likely invalid or redundant comment.
630-633
: 🛠️ Refactor suggestionConsider checking for state change before notifying observers
The added comment explains the rationale for notifying observers after a PUT/POST request. However, notifying observers for every such request might be inefficient if the resource state hasn't actually changed.
Consider applying this diff to check if the resource has actually changed before notifying observers:
/* If the recently handled request was a PUT/POST, it conceivably * altered the resource state, so attempt to notify all observers * of that resource with the change. */ - oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask); + if (resource_state_changed) { + oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask); + }Ensure that
resource_state_changed
is set totrue
only when the resource state is modified.Likely invalid or redundant comment.
72c27a8
to
66ff75f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
api/oc_ri_server.c (4)
69-91
: Consider adding error handling for allocation failuresThe new allocation functions (
oc_ri_alloc_resource
,oc_ri_alloc_resource_defaults
) don't check for allocation failures. In resource-constrained environments, it's crucial to handle these cases.Consider modifying these functions to check for NULL returns and handle the error appropriately. For example:
oc_resource_t * oc_ri_alloc_resource(void) { - return oc_memb_alloc(&g_app_resources_s); + oc_resource_t *resource = oc_memb_alloc(&g_app_resources_s); + if (resource == NULL) { + OC_ERR("Failed to allocate resource"); + } + return resource; }Apply similar changes to
oc_ri_alloc_resource_defaults
.🧰 Tools
🪛 cppcheck
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
93-130
: Consider enhancing error reporting in oc_ri_add_resourceThe
oc_ri_add_resource
function includes comprehensive checks, which is excellent. To further improve its usability, consider enhancing the error reporting by returning specific error codes or using a more detailed logging mechanism.For example:
bool oc_ri_add_resource(oc_resource_t *resource) { if (resource == NULL) { + OC_ERR("Cannot add NULL resource"); return false; } // ... (existing checks) if (oc_ri_URI_is_in_use(resource->device, oc_string(resource->uri), oc_string_len(resource->uri))) { - OC_ERR("resource(%s) URI is already in use", oc_string(resource->uri)); + OC_ERR("Resource URI '%s' is already in use for device %zu", + oc_string(resource->uri), resource->device); return false; } // ... (rest of the function) }This approach provides more context in the error messages, making debugging easier.
527-555
: Enhance error handling and logging in ri_observe_handlerThe
ri_observe_handler
function could benefit from more detailed error handling and logging. Consider the following improvements:
- Add more specific error codes or messages for different failure scenarios.
- Include logging for successful operations to aid in debugging and monitoring.
Here's an example of how you might enhance this function:
static int ri_observe_handler(const coap_packet_t *request, const coap_packet_t *response, oc_resource_t *resource, uint16_t block2_size, const oc_endpoint_t *endpoint, oc_interface_mask_t iface_mask) { if (request->code != COAP_GET || response->code >= 128 || !IS_OPTION(request, COAP_OPTION_OBSERVE)) { + OC_DBG("Invalid observe request: code=%d, response_code=%d, has_observe_option=%d", + request->code, response->code, IS_OPTION(request, COAP_OPTION_OBSERVE)); return -1; } if (request->observe == OC_COAP_OPTION_OBSERVE_REGISTER) { if (NULL == coap_add_observer(resource, block2_size, endpoint, request->token, request->token_len, request->uri_path, request->uri_path_len, iface_mask)) { - OC_ERR("failed to add observer"); + OC_ERR("Failed to add observer for resource %s", oc_string(resource->uri)); return -1; } + OC_DBG("Successfully added observer for resource %s", oc_string(resource->uri)); return 0; } if (request->observe == OC_COAP_OPTION_OBSERVE_UNREGISTER) { if (!coap_remove_observer_by_token(endpoint, request->token, request->token_len)) { + OC_DBG("Observer not found for unregister request on resource %s", oc_string(resource->uri)); return 0; } + OC_DBG("Successfully removed observer for resource %s", oc_string(resource->uri)); return 1; } + OC_DBG("Invalid observe option: %d", request->observe); return -1; }These changes provide more context in logs, making it easier to track and debug observation-related issues.
663-692
: Enhance consistency and error handling in ETag functionsThe ETag-related functions are a good addition, but consider the following improvements:
- Add input validation for the
oc_ri_get_batch_etag
function.- Use consistent naming conventions for the ETag-related macros and functions.
- Consider adding more detailed comments explaining the behavior of
oc_ri_get_batch_etag
for different resource types.Here's an example of how you might enhance these functions:
#ifdef OC_HAS_FEATURE_ETAG uint64_t oc_ri_get_etag(const oc_resource_t *resource) { if (resource == NULL) { OC_ERR("Attempted to get ETag for NULL resource"); return OC_ETAG_UNINITIALIZED; } return resource->etag; } uint64_t oc_ri_get_batch_etag(const oc_resource_t *resource, const oc_endpoint_t *endpoint, size_t device) { if (resource == NULL || endpoint == NULL) { OC_ERR("Invalid input to oc_ri_get_batch_etag"); return OC_ETAG_UNINITIALIZED; } // Handle discovery resource #ifdef OC_RES_BATCH_SUPPORT if (oc_core_get_resource_by_index(OCF_RES, device) == resource) { return oc_discovery_get_batch_etag(endpoint, device); } #endif /* OC_RES_BATCH_SUPPORT */ // Handle collections #ifdef OC_COLLECTIONS if (oc_check_if_collection(resource)) { return oc_collection_get_batch_etag((const oc_collection_t *)resource); } #endif /* OC_COLLECTIONS */ // Handle other resource types OC_DBG("No specific batch ETag handling for resource type, using default"); return OC_ETAG_UNINITIALIZED; } #endif /* OC_HAS_FEATURE_ETAG */These changes improve error handling, add input validation, and provide more context through comments and debug logging.
api/oc_ri.c (2)
1304-1304
: Approve addition of observation handling with suggestion for error handlingThe addition of
oc_ri_handle_observation
is appropriate for managing observation requests on GET methods. This enhances the functionality of the resource interface.Consider adding error handling for the
oc_ri_handle_observation
call:- observe = oc_ri_handle_observation( + int result = oc_ri_handle_observation( ctx->request, ctx->response, cur_resource, get_resource_is_collection(ctx->preparsed_request_obj), block2_size, endpoint, ctx->preparsed_request_obj->iface_query); + if (result < 0) { + OC_ERR("Failed to handle observation request"); + // Consider appropriate error handling + } else { + observe = result; + }This change will help catch and handle any errors that might occur during the observation process.
1360-1360
: Approve server reset and shutdown additions with suggestion for error handlingThe additions of
oc_ri_server_reset();
andoc_ri_server_shutdown();
are appropriate for proper cleanup and shutdown of server components. They are correctly placed within theOC_SERVER
blocks.Consider adding error handling for both function calls:
#ifdef OC_SERVER - oc_ri_server_reset(); + if (oc_ri_server_reset() != 0) { + OC_ERR("Failed to reset OCF Resource Interface server"); + // Consider appropriate error handling + } #endif /* OC_SERVER */ // ... (in oc_ri_shutdown function) #ifdef OC_SERVER - oc_ri_server_shutdown(); + if (oc_ri_server_shutdown() != 0) { + OC_ERR("Failed to shutdown OCF Resource Interface server"); + // Consider appropriate error handling + } #endif /* OC_SERVER */These changes will help detect and handle any errors that might occur during the reset and shutdown processes, improving the robustness of the server management.
Also applies to: 1386-1386
api/oc_discovery.c (2)
Line range hint
1244-1278
: Improve robustness of 'rt' query parameter handling and response construction.There are a few areas in this function that could be improved:
The handling of the 'rt' query parameter could be more robust. Currently, it only checks for exact matches with "oic.wk.res" or the device type. It might be beneficial to support partial matches or multiple 'rt' values.
The construction of the response string is done by directly adding to a buffer. This approach is prone to buffer overflow issues if the constructed string exceeds the buffer size. Consider using a safer string construction method, such as snprintf, or dynamically allocating the buffer based on the required size.
The function assumes that there will always be a secured endpoint available. It might be worth adding a check to handle cases where no secured endpoint is found.
Consider refactoring the response construction to use a safer method:
char response_buffer[MAX_RESPONSE_LENGTH]; int response_length = snprintf(response_buffer, sizeof(response_buffer), "<coap://%s/oic/res>;rt=\"oic.wk.res %s\";if=\"oic.if.ll oic.if.baseline\";ct=10000", endpoint_string, rt_device ? rt_device : ""); if (response_length >= sizeof(response_buffer)) { // Handle buffer overflow return OC_STATUS_INTERNAL_SERVER_ERROR; }Also, consider adding a check for the availability of a secured endpoint:
if (eps == NULL) { OC_ERR("No secured endpoint available for discovery response"); return OC_STATUS_INTERNAL_SERVER_ERROR; }
Line range hint
1-1278
: Overall, the code is well-structured but could benefit from some improvements.The
api/oc_discovery.c
file implements discovery-related functionality for the OCF stack. While the code is generally well-organized and follows consistent practices, there are a few areas where improvements could be made:
Error Handling: Some functions could benefit from more robust error handling, particularly when dealing with memory allocation or buffer operations.
Buffer Management: There are instances where buffer operations could potentially lead to overflows. Consider using safer string handling functions or implementing bounds checking more consistently.
Code Complexity: The extensive use of preprocessor directives for conditional compilation, while providing flexibility, also increases the code's complexity. Consider if some of this complexity could be reduced through alternative design patterns.
Documentation: While the code is generally well-commented, some complex sections could benefit from additional inline documentation explaining the rationale behind certain decisions.
Testing: Given the complexity of the discovery process, it would be beneficial to ensure comprehensive unit tests are in place for this file.
Consider refactoring some of the larger functions (e.g.,
oc_wkcore_discovery_handler
) into smaller, more focused functions to improve readability and maintainability. This could also help in reducing the nesting level of conditional statements in some areas.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- api/cloud/unittest/cloud_store_test.cpp (1 hunks)
- api/oc_collection.c (1 hunks)
- api/oc_discovery.c (2 hunks)
- api/oc_ri.c (5 hunks)
- api/oc_ri_internal.h (0 hunks)
- api/oc_ri_server.c (2 hunks)
- api/oc_ri_server_internal.h (2 hunks)
- include/oc_ri.h (16 hunks)
💤 Files with no reviewable changes (1)
- api/oc_ri_internal.h
🚧 Files skipped from review as they are similar to previous changes (4)
- api/cloud/unittest/cloud_store_test.cpp
- api/oc_collection.c
- api/oc_ri_server_internal.h
- include/oc_ri.h
🧰 Additional context used
🪛 cppcheck
api/oc_ri_server.c
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (4)
api/oc_ri_server.c (3)
20-50
: LGTM: New includes and conditional compilation blocksThe added includes and conditional compilation blocks improve code modularity and feature management. This approach allows for better control over enabled features and dependencies.
52-56
: LGTM: New structure for resource defaultsThe
oc_resource_defaults_data_t
structure is well-defined and appropriate for managing resource defaults. It provides a clean way to associate a resource with its interface mask.
Line range hint
1-693
: Overall assessment: Significant improvements with some suggestions for enhancementThe changes in this file significantly enhance the resource management and observation handling capabilities of the IoTivity stack. Key improvements include:
- Better modularization through new includes and conditional compilation.
- Enhanced resource allocation and management functions.
- Improved observation handling with support for collections.
- Addition of ETag support for resources and batches.
While these changes are generally well-implemented, several suggestions have been made to further improve the code:
- Enhance error handling and logging across various functions.
- Refactor complex functions for better readability and maintainability.
- Address TODO comments, particularly in observation handling.
- Improve consistency in naming conventions and function signatures.
- Add input validation where appropriate.
Implementing these suggestions will further increase the robustness and maintainability of the code. Great work overall on these improvements!
api/oc_ri.c (1)
630-633
: 🛠️ Refactor suggestionConsider checking for state change before notifying observers
The addition of the comment and the call to
oc_ri_notify_resource_observers
is good for maintaining consistency after PUT/POST requests. However, notifying observers when the state hasn't changed could lead to unnecessary network traffic.Consider modifying the code to check if the resource state has actually changed:
/* If the recently handled request was a PUT/POST, it conceivably * altered the resource state, so attempt to notify all observers * of that resource with the change. */ - oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask); + if (resource_state_changed) { + oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask); + }This change assumes you have a way to track if the resource state has changed during the request handling. If not, you might need to implement such a mechanism.
Likely invalid or redundant comment.
66ff75f
to
4cbd9dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
port/linux/ipadapter.c (1)
Line range hint
92-105
: Robustness inadd_ip_interface
FunctionThe
add_ip_interface
function adds a new interface to the list. Consider the following suggestions:
- Check Memory Allocation: You correctly check if
new_if
isNULL
after attempting to allocate memory. Good job.- Thread Safety: If this function can be called from multiple threads, consider adding mutex locks around the list operations to ensure thread safety.
♻️ Duplicate comments (1)
api/oc_buffer_settings.c (1)
69-73
:⚠️ Potential issueUse size_t for size-related return values.
The functions return
long
but operate onsize_t
values. This could lead to truncation on platforms wheresize_t
is larger thanlong
.Apply this diff to fix the return types:
-long +size_t oc_get_mtu_size(void) { - return (long)_OC_MTU_SIZE; + return _OC_MTU_SIZE; }Also applies to: 92-96, 108-112, 114-118
🧹 Nitpick comments (11)
port/linux/ipadapter.c (7)
Line range hint
75-90
: Implementation ofget_ip_interface
FunctionThe
get_ip_interface
function correctly searches theg_ip_interface_list
for a matching interface index. However, consider the following improvements:
- Check for Null Pointers: Although
oc_list_head
should not returnNULL
in normal conditions, it's good practice to check forNULL
to prevent potential dereference issues.- Use Consistent Variable Naming: Use consistent naming conventions for variables. For example, you could rename
if_item
tointerface_item
for clarity.
Line range hint
107-122
: Error Handling incheck_new_ip_interfaces
FunctionThe function
check_new_ip_interfaces
gathers network interfaces. Recommendations:
- Handle Errors from
getifaddrs
: You correctly handle the error whengetifaddrs
fails.- Free Resources: Ensure that
freeifaddrs(ifs);
is called in all code paths to prevent memory leaks.- Interface Flags: You correctly check if the interface is up and not a loopback. Consider also checking for
IFF_RUNNING
to ensure the interface is fully operational.
Line range hint
124-139
: Improveremove_ip_interface
FunctionThe
remove_ip_interface
function removes an interface from the list. Suggestions:
- Check Return Values: After calling
oc_list_remove
, it's good practice to verify that the item was indeed removed.- Logging: The debug log "Removed from ip interface list" is helpful. Ensure that logging is consistent across similar functions.
Line range hint
141-152
: Efficiently Implementremove_all_ip_interface
FunctionThe function correctly iterates through the list and frees each item. Consider the following:
- Optimize Loop: Since you're removing all items, you might use
oc_list_for_each_safe
if available, to simplify the loop.-static void -remove_all_ip_interface(void) -{ - ip_interface_t *if_item = oc_list_head(g_ip_interface_list); - while (if_item != NULL) { - ip_interface_t *next = if_item->next; - oc_list_remove(g_ip_interface_list, if_item); - oc_memb_free(&g_ip_interface_s, if_item); - if_item = next; - } -} +static void +remove_all_ip_interface(void) +{ + ip_interface_t *if_item = NULL; + ip_interface_t *next = NULL; + oc_list_for_each_safe(if_item, g_ip_interface_list, next) { + oc_list_remove(g_ip_interface_list, if_item); + oc_memb_free(&g_ip_interface_s, if_item); + } +}
Line range hint
154-166
: Consistency inremove_all_network_interface_cbs
FunctionThe function removes all network interface callbacks. Suggestions:
- Thread Safety: If callbacks can be added or removed from different threads, consider adding synchronization mechanisms.
- Code Reuse: The pattern here is similar to
remove_all_ip_interface
. Consider abstracting this pattern into a helper function for code reuse.
Line range hint
442-465
: Handle Network Interface Changes Robustly inprocess_interface_change_event
The function
process_interface_change_event
processes network interface changes. Recommendations:
- Error Checking: You handle errors from
get_data
and check forNLMSG_ERROR
. Good practice.- Logic for Interface Events: When handling
RTM_NEWADDR
andRTM_DELADDR
, consider additional checks for interface validity.- Thread Safety: Ensure that modifications to shared data structures are thread-safe.
Line range hint
783-807
: Correct Usage ofFD_SET
andselect
innetwork_event_thread
In the
network_event_thread
, you correctly prepare the file descriptor sets forselect
. Suggestions:
- Manage
FD_SETSIZE
Limitations: Be cautious withFD_SETSIZE
, which may limit the number of file descriptors your application can handle.- Error Handling: Check the return value of
select
for errors and handle them appropriately.include/oc_buffer_settings.h (3)
38-46
: Consider adding parameter validation and constraints documentation.While the MTU size functions are well-structured, consider:
- Adding
OC_NONNULL()
for pointer parameters if applicable- Documenting valid size ranges and constraints
- Clarifying error return values for
oc_set_mtu_size
54-62
: Document size constraints for application data settings.For both max and min app data size functions:
- Document the relationship between min and max sizes
- Specify any platform-specific limitations
- Consider adding validation to ensure min ≤ max
Also applies to: 71-79
87-88
: Consider documenting block size calculation.The
oc_get_block_size
function would benefit from documentation explaining:
- How the block size is calculated
- Its relationship with other buffer settings
- Whether it's configurable or derived
include/oc_session_events.h (1)
117-132
: Consider adding error handling example.The documentation for
oc_remove_session_event_callback_v1
could benefit from an example showing how to handle the different return values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
api/cloud/oc_cloud.c
(1 hunks)api/cloud/unittest/cloud_store_test.cpp
(1 hunks)api/oc_buffer_settings.c
(1 hunks)api/oc_discovery.c
(2 hunks)api/oc_helpers.c
(4 hunks)api/oc_main.c
(0 hunks)api/oc_session_events.c
(0 hunks)api/plgd/device-provisioning-client/plgd_dps.c
(2 hunks)api/plgd/device-provisioning-client/plgd_dps_manager.c
(0 hunks)api/plgd/unittest/plgdtimetest.cpp
(0 hunks)api/unittest/buffersettingstest.cpp
(1 hunks)api/unittest/helperstest.cpp
(1 hunks)include/oc_buffer_settings.h
(3 hunks)include/oc_enums.h
(5 hunks)include/oc_network_monitor.h
(2 hunks)include/oc_session_events.h
(2 hunks)include/oc_signal_event_loop.h
(1 hunks)include/oc_sp.h
(2 hunks)port/android/ipadapter.c
(1 hunks)port/esp32/adapter/src/ipadapter.c
(1 hunks)port/esp32/main/CMakeLists.txt
(1 hunks)port/linux/ipadapter.c
(1 hunks)port/oc_connectivity.h
(1 hunks)port/oc_storage.h
(2 hunks)port/unittest/connectivitytest.cpp
(2 hunks)port/windows/ipadapter.c
(1 hunks)port/windows/vs2015/IoTivity-lite.vcxproj
(1 hunks)port/windows/vs2015/IoTivity-lite.vcxproj.filters
(1 hunks)swig/swig_interfaces/oc_buffer_settings.i
(1 hunks)swig/swig_interfaces/oc_enums.i
(1 hunks)
💤 Files with no reviewable changes (4)
- api/oc_session_events.c
- api/plgd/device-provisioning-client/plgd_dps_manager.c
- api/plgd/unittest/plgdtimetest.cpp
- api/oc_main.c
✅ Files skipped from review due to trivial changes (1)
- api/cloud/oc_cloud.c
🚧 Files skipped from review as they are similar to previous changes (2)
- api/cloud/unittest/cloud_store_test.cpp
- api/oc_discovery.c
🧰 Additional context used
🪛 GitHub Actions: Static analysis
port/esp32/main/CMakeLists.txt
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
[warning] 416-416: Cannot set PLGD_DEV_DEVICE_PROVISIONING_ENABLED with OC_OSCORE_ENABLED
port/oc_connectivity.h
[error] 32-32: Missing include file: 'port/oc_dns.h' file not found
🪛 GitHub Actions: PR #647
port/esp32/main/CMakeLists.txt
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
port/oc_connectivity.h
[error] 32-32: Missing include file: port/oc_dns.h: No such file or directory
🪛 GitHub Actions: CMake Windows
port/esp32/main/CMakeLists.txt
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
port/oc_connectivity.h
[error] 32-32: Missing include file: 'port/oc_dns.h' file not found
🪛 GitHub Actions: Create and publish docker images
port/esp32/main/CMakeLists.txt
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
port/oc_connectivity.h
[error] 32-32: Missing header file 'port/oc_dns.h' - Include file not found during compilation
🪛 GitHub Actions: CI building android
port/oc_connectivity.h
[error] 32-32: Missing include file: 'port/oc_dns.h' file not found
🔇 Additional comments (34)
port/linux/ipadapter.c (6)
43-45
: Conditional Inclusion ofoc_network_monitor.h
Good job on conditionally including
oc_network_monitor.h
based on theOC_NETWORK_MONITOR
macro. This ensures that the network monitoring features are only included when necessary, improving modularity.
Line range hint
67-70
: Defineip_interface_t
Structure AppropriatelyThe definition of
ip_interface_t
is placed within the#ifdef OC_NETWORK_MONITOR
block, which is correct. Ensure that any usage ofip_interface_t
outside this block is also conditionally compiled to prevent compilation errors whenOC_NETWORK_MONITOR
is not defined.
Line range hint
72-73
: Initialize Lists and Memory PoolsThe initialization of
g_ip_interface_list
andg_ip_interface_s
usingOC_LIST
andOC_MEMB
macros is appropriate. This allows for efficient management of theip_interface_t
structures.
Line range hint
214-216
: Ensure Proper Resource Release inoc_network_event_handler_mutex_destroy
You correctly call
remove_all_ip_interface()
andremove_all_network_interface_cbs()
within#ifdef OC_NETWORK_MONITOR
. Ensure that any resources allocated whenOC_NETWORK_MONITOR
is defined are appropriately released here.
Line range hint
1066-1083
: Resource Cleanup inoc_connectivity_shutdown
In the
oc_connectivity_shutdown
function, you close sockets and free resources. Recommendations:
- Close All Open File Descriptors: Ensure that all sockets and file descriptors are closed, including any that may have been added in future modifications.
- Mutex Destruction: You correctly destroy the mutex
rfds_mutex
. Ensure that any other synchronization primitives are also destroyed.
Line range hint
1085-1093
: Functionality ofoc_connectivity_end_session
The function
oc_connectivity_end_session
appears to loop whileoc_connectivity_end_session_v1
returnstrue
. Ensure that this does not lead to an infinite loop in edge cases.Please confirm that
oc_connectivity_end_session_v1
will eventually returnfalse
to prevent an infinite loop.swig/swig_interfaces/oc_buffer_settings.i (1)
25-26
: Include Macros for API Visibility and DeprecationDefining
#define OC_API
and#define OC_DEPRECATED(...)
is appropriate to handle API export definitions and deprecation attributes, especially when generating language bindings with SWIG.Ensure that these definitions are compatible with those used in the corresponding header files to prevent linkage or compilation issues.
swig/swig_interfaces/oc_enums.i (1)
35-36
: Define Macros for API and Non-Null AnnotationsAdding
#define OC_API
and#define OC_NONNULL(...)
is appropriate to handle API visibility and non-null annotations in the SWIG interface file.Make sure these macros align with their usage in the included header files to maintain consistency and prevent compilation warnings or errors.
include/oc_signal_event_loop.h (1)
24-25
: LGTM! Changes align with PR objectives.The addition of the
oc_export.h
include andOC_API
annotation to_oc_signal_event_loop
is correct and consistent with the goal of properly marking exported APIs.Also applies to: 33-33
include/oc_network_monitor.h (1)
38-38
: LGTM! API annotations properly added.The
OC_API
annotations are correctly added to the network interface event callback functions. Documentation is clear and comprehensive.Also applies to: 49-49
port/oc_storage.h (1)
24-26
: LGTM! Enhanced API safety with proper annotations.The changes improve the API by:
- Adding proper export annotations
- Adding compile-time parameter validation with
OC_NONNULL()
- Clarifying documentation about NULL constraints
Also applies to: 41-41, 52-53, 64-66
include/oc_buffer_settings.h (1)
24-25
: LGTM! Required header properly included.The addition of
oc_export.h
is necessary for theOC_API
annotations.api/unittest/buffersettingstest.cpp (1)
1-90
: LGTM! Well-structured test suite.The test coverage is comprehensive, testing both dynamic and static allocation scenarios. Good practice to restore initial values after modification.
include/oc_session_events.h (2)
80-91
: Good use of deprecation notice.The deprecation notice clearly indicates the replacement function and version.
93-103
: Well-documented API enhancement.The new callback function with user data support improves flexibility while maintaining backward compatibility.
api/oc_buffer_settings.c (2)
41-41
: Remove or address the "FIX" comment.The comment
// FIX
suggests that this value needs attention. Either justify the value or implement the proper calculation.
51-64
: Consider validating block size calculation.The block size calculation uses bit shifting without explicit bounds checking. Consider adding validation to ensure the calculated block size is within acceptable limits.
✅ Verification successful
Block size calculation is properly validated and bounded.
The block size calculation is safe because:
- Input MTU is validated against minimum size (COAP_MAX_HEADER_SIZE + 16)
- Block size is constrained to power-of-2 values through bit shifting
- Block size is bounded by OC_MAX_BLOCK_SIZE which is MTU minus header
- Additional runtime validation exists when used with peer MTU
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential block size validation in other parts of the codebase rg -A 5 "block.*size.*valid|valid.*block.*size"Length of output: 65586
Script:
#!/bin/bash # Search for OC_BLOCK_SIZE usage and validation rg -A 5 "_OC_BLOCK_SIZE|OC_BLOCK_SIZE" # Search for block size related constants and limits rg -A 2 "BLOCK_SIZE|block_size.*max|max.*block_size|block_size.*min|min.*block_size" # Search for block size validation in test files fd -e c -e h "test" --exec rg -A 5 "block.*size|_OC_BLOCK_SIZE" {}Length of output: 76283
include/oc_enums.h (2)
206-207
: LGTM! Well-documented enum conversion functions.The API is clear and follows consistent patterns for string conversion functions.
Also applies to: 215-216, 224-225
235-237
: Good use of OC_NONNULL annotation.The OC_NONNULL annotation properly enforces that the oc_defined parameter cannot be NULL.
include/oc_sp.h (1)
28-28
: LGTM! Appropriate use of OC_API macro.The changes correctly expose the
oc_pki_set_security_profile
function as part of the public API by adding theOC_API
macro and including the required header.Also applies to: 131-131
api/oc_helpers.c (1)
315-315
: LGTM! Good use of safer string functions.The changes from
strlen
tooc_strnlen
improve safety by adding length limits to string operations, preventing buffer overflows.Also applies to: 355-355
api/unittest/helperstest.cpp (1)
259-341
: LGTM! Comprehensive test coverage.The new test cases provide thorough coverage of the hex string conversion functions:
- Error cases: buffer too small, empty input
- Success cases: even and odd length hex strings
- Edge cases: buffer size checks
api/plgd/device-provisioning-client/plgd_dps.c (3)
33-36
: LGTM! Clean header inclusion.The conditional inclusion of the network monitor header is properly guarded with the
OC_NETWORK_MONITOR
macro.
232-234
: LGTM! Clean callback registration.The network interface event callback registration is properly guarded and follows the initialization pattern.
240-242
: LGTM! Clean callback deregistration.The network interface event callback deregistration is properly guarded and ensures proper cleanup.
port/unittest/connectivitytest.cpp (2)
36-39
: LGTM! Clean header inclusion.The conditional inclusion of the network monitor header is properly guarded.
Line range hint
163-193
: LGTM! Comprehensive test coverage.The network interface event tests are well-structured and cover:
- Event handler implementation
- Callback registration
- Callback removal
- Failed callback removal
- Event handling
port/windows/vs2015/IoTivity-lite.vcxproj (1)
357-357
: LGTM! Clean project file update.The new source file is properly added to the project.
port/windows/vs2015/IoTivity-lite.vcxproj.filters (1)
116-118
: LGTM! Clean filter update.The new source file is properly added to the Core filter.
port/windows/ipadapter.c (1)
38-40
: LGTM: Good use of conditional compilation for optional feature.The addition of the
#ifdef OC_NETWORK_MONITOR
directive for includingoc_network_monitor.h
is a good practice. It allows for conditional compilation of network monitoring features, improving modularity and flexibility of the codebase.port/android/ipadapter.c (1)
43-45
: LGTM: Consistent use of conditional compilation across platform adapters.The conditional inclusion of
oc_network_monitor.h
matches the implementation in other adapter files.port/esp32/adapter/src/ipadapter.c (1)
36-38
: LGTM: Consistent use of conditional compilation across platform adapters.The conditional inclusion of
oc_network_monitor.h
matches the implementation in other adapter files.port/esp32/main/CMakeLists.txt (2)
33-33
: LGTM: New source file added for buffer settings management.The addition of
${CMAKE_CURRENT_SOURCE_DIR}/../../../api/oc_buffer_settings.c
to the list of source files is appropriate.🧰 Tools
🪛 GitHub Actions: Static analysis
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
🪛 GitHub Actions: PR #647
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
🪛 GitHub Actions: CMake Windows
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
🪛 GitHub Actions: Create and publish docker images
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
33-33
: Consider upgrading MbedTLS version.The pipeline indicates that MbedTLS v3.1.0 is deprecated and support will be removed in a future release.
Run the following script to check the current MbedTLS version configuration:
✅ Verification successful
Confirmed: MbedTLS v3.1.0 is deprecated and should be upgraded.
The codebase currently uses MbedTLS v3.1.0 by default, which is marked as deprecated. You can upgrade to v3.5.0 by setting the CMake option
BUILD_MBEDTLS_FORCE_3_5_0=ON
. The project already includes the necessary patches for v3.5.0.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check MbedTLS version configuration # Search for MbedTLS version configuration rg -l "MBEDTLS.*VERSION" --type cmakeLength of output: 39
Script:
#!/bin/bash # Search for MbedTLS related configurations and dependencies # Search for mbedtls references (case-insensitive) echo "=== Searching for mbedtls references ===" rg -i "mbedtls" --type cmake # Search for mbedtls in any pipeline or CI configurations echo -e "\n=== Searching for CI configurations ===" fd -e yml -e yaml | xargs rg -i "mbedtls" || true # Search for version requirements or dependencies echo -e "\n=== Searching for version requirements ===" fd CMakeLists.txt | xargs rg -i "find_package.*mbedtls|require.*mbedtls"Length of output: 8967
🧰 Tools
🪛 GitHub Actions: Static analysis
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
🪛 GitHub Actions: PR #647
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
🪛 GitHub Actions: CMake Windows
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
🪛 GitHub Actions: Create and publish docker images
[warning] 33-33: MbedTLS v3.1.0 is deprecated and support will be removed in a future release
4cbd9dc
to
002e89a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (14)
api/unittest/buffersettingstest.cpp (4)
27-37
: Consider adding more test cases for MTU size validation.The test only checks for invalid MTU size (42). Consider adding test cases for:
- Edge cases (minimum and maximum valid MTU sizes)
- Common MTU sizes (1280, 1500)
41-48
: Add validation for max app data size.The test doesn't verify that setting invalid sizes (e.g., 0 or negative values) is handled correctly.
TEST(TestBufferSettings, SetMaxAppDataSize) { auto max_app_size = static_cast<size_t>(oc_get_max_app_data_size()); + oc_set_max_app_data_size(0); + EXPECT_EQ(-1, oc_get_max_app_data_size()); oc_set_max_app_data_size(42); EXPECT_EQ(42, oc_get_max_app_data_size()); oc_set_max_app_data_size(max_app_size); }
52-59
: Add validation for min app data size.Similar to max app data size, the test should verify handling of invalid sizes.
TEST(TestBufferSettings, SetMinAppDataSize) { auto min_app_size = static_cast<size_t>(oc_get_min_app_data_size()); + oc_set_min_app_data_size(0); + EXPECT_EQ(-1, oc_get_min_app_data_size()); oc_set_min_app_data_size(42); EXPECT_EQ(42, oc_get_min_app_data_size()); oc_set_min_app_data_size(min_app_size); }
85-88
: Add more test cases for block size.The test only verifies that
oc_get_block_size
returns -1 when dynamic allocation is disabled. Consider adding test cases for:
- Block size calculation based on MTU size
- Minimum and maximum block sizes
api/oc_buffer_settings.c (1)
51-64
: Document the block size calculation logic.The block size calculation using bit shifts needs documentation explaining the rationale behind the algorithm.
Add a comment explaining the block size calculation:
size_t i; + // Find the largest power of 2 that fits within the MTU size + // by checking each bit position from 10 down to 4 for (i = 10; i >= 4 && (mtu_size >> i) == 0; i--) ; _OC_BLOCK_SIZE = ((size_t)1) << i;api/unittest/helperstest.cpp (5)
260-270
: LGTM! Consider enhancing error validation.The test correctly verifies buffer overflow protection. Consider adding assertions to verify that the buffer content remains unmodified when the error occurs.
TEST(Helpers, HexStringTooSmallError) { std::array<uint8_t, 4> array{ 0x1f, 0xa0, 0x5b, 0xff }; std::array<char, 5> hex_str; // Too small to fit the full hex representation // (8 chars + null terminator) + hex_str.fill('x'); // Initialize with known value size_t hex_str_len = hex_str.size(); int result = oc_conv_byte_array_to_hex_string(array.data(), array.size(), &hex_str[0], &hex_str_len); ASSERT_EQ(result, -1); + // Verify buffer wasn't modified + EXPECT_TRUE(std::all_of(hex_str.begin(), hex_str.end(), + [](char c) { return c == 'x'; })); }
272-284
: LGTM! Consider adding boundary value tests.The test effectively verifies the successful conversion path. Consider adding test cases for boundary values (e.g., all zeros, all ones) to ensure robust conversion.
+TEST(Helpers, ConvertByteArrayToHexStringBoundaryValues) +{ + std::array<char, 9> hex_str; + size_t hex_str_len = hex_str.size(); + + // Test all zeros + std::array<uint8_t, 4> zeros{0x00, 0x00, 0x00, 0x00}; + ASSERT_EQ(oc_conv_byte_array_to_hex_string(zeros.data(), zeros.size(), + &hex_str[0], &hex_str_len), 0); + EXPECT_STREQ(hex_str.data(), "00000000"); + + // Test all ones + std::array<uint8_t, 4> ones{0xff, 0xff, 0xff, 0xff}; + ASSERT_EQ(oc_conv_byte_array_to_hex_string(ones.data(), ones.size(), + &hex_str[0], &hex_str_len), 0); + EXPECT_STREQ(hex_str.data(), "ffffffff"); +}
286-296
: LGTM! Consider enhancing error validation.The test correctly verifies empty input handling. Consider adding assertions to verify that the output buffer remains unmodified when the error occurs.
TEST(Helpers, EmptyHexStringError) { std::string hex_str = ""; std::array<uint8_t, 4> array; + array.fill(0xAA); // Initialize with known value size_t array_len = array.size(); int result = oc_conv_hex_string_to_byte_array( hex_str.c_str(), hex_str.length(), &array[0], &array_len); ASSERT_EQ(result, -1); + // Verify buffer wasn't modified + EXPECT_TRUE(std::all_of(array.begin(), array.end(), + [](uint8_t b) { return b == 0xAA; })); }
298-309
: LGTM! Consider enhancing error validation.The test correctly verifies buffer overflow protection. Consider adding assertions to verify that the output buffer remains unmodified when the error occurs.
TEST(Helpers, ArrayBufferTooSmallError) { std::string hex_str = "1fa05bff"; std::array<uint8_t, 2> array; // Too small to hold the expected byte array (needs 4 bytes) + array.fill(0xAA); // Initialize with known value size_t array_len = array.size(); int result = oc_conv_hex_string_to_byte_array( hex_str.c_str(), hex_str.length(), &array[0], &array_len); ASSERT_EQ(result, -1); + // Verify buffer wasn't modified + EXPECT_TRUE(std::all_of(array.begin(), array.end(), + [](uint8_t b) { return b == 0xAA; })); }
259-341
: Consider adding tests for additional edge cases.The test suite provides good coverage of the main scenarios. Consider adding tests for:
- Invalid hex characters in the input string (e.g., "1g2h3i4j")
- Null pointer handling for both input and output buffers
- Zero-length array with non-null pointer
TEST(Helpers, InvalidHexCharactersError) { std::string hex_str = "1g2h3i4j"; // Contains invalid hex chars std::array<uint8_t, 4> array; size_t array_len = array.size(); int result = oc_conv_hex_string_to_byte_array( hex_str.c_str(), hex_str.length(), &array[0], &array_len); ASSERT_EQ(result, -1); } TEST(Helpers, NullPointerError) { std::string hex_str = "1234"; std::array<uint8_t, 2> array; size_t array_len = array.size(); // Test null input string int result = oc_conv_hex_string_to_byte_array( nullptr, 4, &array[0], &array_len); ASSERT_EQ(result, -1); // Test null output buffer result = oc_conv_hex_string_to_byte_array( hex_str.c_str(), hex_str.length(), nullptr, &array_len); ASSERT_EQ(result, -1); } TEST(Helpers, ZeroLengthArrayError) { uint8_t buffer[1]; // Non-null buffer size_t buffer_len = 0; // Zero length std::string hex_str = "12"; int result = oc_conv_hex_string_to_byte_array( hex_str.c_str(), hex_str.length(), buffer, &buffer_len); ASSERT_EQ(result, -1); }port/oc_dns.h (3)
19-22
: Enhance file description documentation.The file description section is empty. Consider adding a comprehensive description that explains:
- The purpose of this header file
- The DNS functionality it provides
- Usage requirements and dependencies
/** * @file + * @brief DNS operations for IoTivity-lite + * + * This header file provides DNS lookup and caching functionality for IoTivity-lite. + * It supports domain name resolution and optional DNS caching when the respective + * features (OC_DNS_LOOKUP, OC_DNS_CACHE) are enabled. */
37-47
: Enhance function documentation foroc_dns_lookup
.The current documentation lacks important details. Consider adding:
- Detailed parameter descriptions including constraints and formats
- Complete return value documentation including error codes
- Example usage
/** * @brief dns look up + * + * Performs DNS resolution for the given domain name using the specified transport. * - * @param domain the url - * @param addr the address - * @param flags the transport flags - * @return int 0 = success + * @param domain The domain name to resolve (e.g., "example.com") + * @param addr Output parameter to store the resolved address + * @param flags Transport flags specifying the protocol requirements + * + * @return 0 on successful resolution + * @return -1 on invalid parameters or resolution failure + * + * @note The resolved address is stored in the provided oc_string_t structure + * which must be initialized before calling this function. + * + * Example: + * @code + * oc_string_t addr; + * oc_string_init(&addr); + * int ret = oc_dns_lookup("example.com", &addr, IPV4|TCP); + * if (ret == 0) { + * // Use resolved address + * } + * oc_string_free(&addr); + * @endcode */
50-54
: Enhance function documentation foroc_dns_clear_cache
.The current documentation lacks important details about cache behavior and usage.
/** * @brief clear the DNS cache + * + * Clears all cached DNS resolutions, forcing future lookups to query DNS servers. + * This can be useful when: + * - Network configuration changes + * - DNS records might have changed + * - Troubleshooting connectivity issues + * + * @note This operation may impact performance of subsequent DNS lookups + * as they will need to query DNS servers instead of using cached results. */include/oc_network_monitor.h (1)
38-38
: LGTM! Consider enhancing the documentation.The API functions are well-defined with proper OC_API annotations and error handling. The documentation is clear but could be enhanced by adding a note about thread safety.
Consider adding thread safety information to the documentation:
/** * @brief Add the callback to receive change notifications for Network interface * event. * @param cb The callback to be added. Must not be NULL. + * @note This function is thread-safe and can be called from any thread. * @return 0 on success * @return -1 on error */
Also applies to: 49-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
api/cloud/oc_cloud.c
(1 hunks)api/cloud/unittest/cloud_store_test.cpp
(1 hunks)api/oc_buffer_settings.c
(1 hunks)api/oc_discovery.c
(2 hunks)api/oc_helpers.c
(4 hunks)api/oc_main.c
(0 hunks)api/oc_session_events.c
(0 hunks)api/plgd/device-provisioning-client/plgd_dps.c
(2 hunks)api/plgd/device-provisioning-client/plgd_dps_manager.c
(0 hunks)api/plgd/unittest/plgdtimetest.cpp
(0 hunks)api/unittest/buffersettingstest.cpp
(1 hunks)api/unittest/helperstest.cpp
(1 hunks)include/oc_buffer_settings.h
(3 hunks)include/oc_enums.h
(5 hunks)include/oc_network_monitor.h
(2 hunks)include/oc_session_events.h
(2 hunks)include/oc_signal_event_loop.h
(1 hunks)include/oc_sp.h
(2 hunks)port/android/ipadapter.c
(1 hunks)port/esp32/adapter/src/ipadapter.c
(1 hunks)port/esp32/main/CMakeLists.txt
(1 hunks)port/linux/ipadapter.c
(1 hunks)port/oc_connectivity.h
(1 hunks)port/oc_dns.h
(1 hunks)port/oc_storage.h
(2 hunks)port/unittest/connectivitytest.cpp
(2 hunks)port/windows/ipadapter.c
(1 hunks)port/windows/vs2015/IoTivity-lite.vcxproj
(2 hunks)port/windows/vs2015/IoTivity-lite.vcxproj.filters
(2 hunks)swig/swig_interfaces/oc_buffer_settings.i
(1 hunks)swig/swig_interfaces/oc_enums.i
(1 hunks)
💤 Files with no reviewable changes (4)
- api/oc_session_events.c
- api/plgd/device-provisioning-client/plgd_dps_manager.c
- api/plgd/unittest/plgdtimetest.cpp
- api/oc_main.c
🚧 Files skipped from review as they are similar to previous changes (19)
- port/esp32/main/CMakeLists.txt
- api/cloud/unittest/cloud_store_test.cpp
- api/cloud/oc_cloud.c
- swig/swig_interfaces/oc_enums.i
- swig/swig_interfaces/oc_buffer_settings.i
- include/oc_signal_event_loop.h
- api/oc_discovery.c
- include/oc_sp.h
- port/windows/vs2015/IoTivity-lite.vcxproj.filters
- api/oc_helpers.c
- port/oc_connectivity.h
- port/windows/ipadapter.c
- port/unittest/connectivitytest.cpp
- include/oc_buffer_settings.h
- port/oc_storage.h
- api/plgd/device-provisioning-client/plgd_dps.c
- port/esp32/adapter/src/ipadapter.c
- include/oc_session_events.h
- include/oc_enums.h
🔇 Additional comments (13)
api/oc_buffer_settings.c (4)
35-41
: Simplify application data size initialization.The initialization of
_OC_MAX_APP_DATA_SIZE
and_OC_MIN_APP_DATA_SIZE
has redundancy.
41-41
: Address the FIX comment for_OC_BLOCK_SIZE
.The line contains a FIX comment that needs to be addressed.
69-73
: Update return types for consistency.The getter functions return
long
while working withsize_t
variables. This could lead to truncation on some platforms.Also applies to: 92-96, 108-112, 114-118
122-170
: Improve error messages for non-dynamic allocation.The warning messages could be more informative.
port/windows/vs2015/IoTivity-lite.vcxproj (1)
358-358
: LGTM!The buffer settings files are correctly added to the project configuration.
Also applies to: 325-325
api/unittest/helperstest.cpp (2)
311-325
: LGTM! The test case is well-designed.The test effectively verifies the successful conversion path with comprehensive assertions checking both the result code and the converted values.
327-341
: LGTM! The test case is well-designed.The test effectively verifies the successful conversion path for odd-length input with comprehensive assertions checking both the result code and the converted values.
port/oc_dns.h (3)
23-30
: LGTM!The include guard and header dependencies are properly organized and follow the project's conventions.
1-64
: Overall, the header is well-structured and modular.The header provides a clean API for DNS operations with good separation of concerns through feature flags. While documentation could be enhanced, the implementation follows good practices and maintains compatibility with C++.
35-58
: Document feature flags and verify dependencies.Consider adding documentation for the feature flags and their relationships.
Let's verify the feature flag dependencies and documentation:
Add documentation for the feature flags:
+/** + * @def OC_DNS_LOOKUP + * Enable DNS resolution support + */ + +/** + * @def OC_DNS_CACHE + * Enable DNS caching support. Requires OC_DNS_LOOKUP to be defined. + */ + #ifdef OC_DNS_LOOKUP✅ Verification successful
Documentation improvements needed for feature flags
The feature flags are correctly implemented with proper dependencies, but documentation should be added to explain their purpose:
/** * @def OC_DNS_LOOKUP * Enable DNS resolution support for converting domain names to IP addresses. */ /** * @def OC_DNS_CACHE * Enable DNS caching to store resolved addresses. Requires OC_DNS_LOOKUP. */🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check feature flag documentation and dependencies # Check if feature flags are documented echo "Checking for feature flag documentation..." fd -t f -e h -e c . | xargs rg -l 'OC_DNS_LOOKUP|OC_DNS_CACHE' | while read -r file; do echo "=== $file ===" rg -A 5 -B 5 'OC_DNS_LOOKUP|OC_DNS_CACHE' "$file" done # Check if OC_DNS_CACHE requires OC_DNS_LOOKUP echo "Checking feature flag dependencies..." rg '#ifdef\s+OC_DNS_CACHE' -A 2 -B 2Length of output: 14468
port/linux/ipadapter.c (2)
43-46
: LGTM! Clean header inclusion.The conditional inclusion of the network monitor header is properly implemented with clear preprocessor guards.
43-45
:⚠️ Potential issueAdd mutex protection for list operations.
The network interface list operations are not thread-safe. Consider adding mutex protection to prevent race conditions when multiple threads access the interface list.
Let's verify the thread safety concerns:
Also applies to: 46-46
port/android/ipadapter.c (1)
43-45
: Apply the same improvements as linux/ipadapter.c.The same issues regarding thread safety, error handling, and memory management exist in this file. Please refer to the review comments for
port/linux/ipadapter.c
and apply the same improvements here.
002e89a
to
d41d4cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
port/linux/ipadapter.c (1)
Line range hint
136-167
: Fix potential memory leak in check_new_ip_interfaces.The function should free the ifaddrs structure before returning on error paths.
static bool check_new_ip_interfaces(void) { struct ifaddrs *ifs = NULL; if (getifaddrs(&ifs) < 0) { OC_ERR("failed querying interface address"); return false; } for (struct ifaddrs *interface = ifs; interface != NULL; interface = interface->ifa_next) { /* Ignore interfaces that are down and the loopback interface */ if ((interface->ifa_flags & IFF_UP) == 0 || (interface->ifa_flags & IFF_LOOPBACK) != 0) { continue; } /* Obtain interface index for this address */ unsigned if_index = if_nametoindex(interface->ifa_name); if (if_index == 0) { OC_ERR("failed obtaining interface(%s) index: %d", interface->ifa_name, (int)errno); + freeifaddrs(ifs); continue; } add_ip_interface(if_index); } freeifaddrs(ifs); return true; }
🧹 Nitpick comments (8)
port/linux/ipadapter.c (2)
Line range hint
116-134
: Improve error handling in add_ip_interface.The error handling could be enhanced with more detailed error information.
static bool add_ip_interface(unsigned target_index) { if (get_ip_interface(target_index)) { + OC_DBG("interface %d already exists", target_index); return false; } ip_interface_t *new_if = oc_memb_alloc(&g_ip_interface_s); if (new_if == NULL) { - OC_ERR("interface item alloc failed"); + OC_ERR("interface item alloc failed for interface %d: %s", target_index, strerror(errno)); return false; } new_if->if_index = target_index; oc_list_add(g_ip_interface_list, new_if); - OC_DBG("New interface added: %d", new_if->if_index); + OC_DBG("New interface added: %d (total interfaces: %d)", new_if->if_index, + oc_list_length(g_ip_interface_list)); return true; }
Line range hint
169-211
: Add input validation and improve documentation for interface management functions.The functions would benefit from additional validation and documentation of their behavior.
+/** + * Remove an interface from the interface list. + * + * @param target_index The index of the interface to remove + * @return true if the interface was found and removed, false otherwise + */ static bool remove_ip_interface(unsigned target_index) { + if (target_index == 0) { + OC_ERR("invalid interface index: %d", target_index); + return false; + } + ip_interface_t *if_item = get_ip_interface(target_index); if (!if_item) { + OC_DBG("interface %d not found", target_index); return false; } oc_list_remove(g_ip_interface_list, if_item); oc_memb_free(&g_ip_interface_s, if_item); OC_DBG("Removed from ip interface list: %d", target_index); return true; }api/unittest/helperstest.cpp (4)
272-284
: Consider adding null termination check.The test is well-designed but could be more robust by verifying that the output string is properly null-terminated.
Add this assertion:
ASSERT_EQ(result, 0); EXPECT_STREQ(hex_str.data(), "1fa05bff"); EXPECT_EQ(hex_str_len, 8); + EXPECT_EQ(hex_str[8], '\0');
286-296
: Consider validating output buffer integrity.While the test correctly checks the error code, it would be more thorough to verify that the output buffer remains unmodified when the conversion fails.
Add these assertions:
std::array<uint8_t, 4> array; + array.fill(0xAA); // Initialize with known pattern size_t array_len = array.size(); int result = oc_conv_hex_string_to_byte_array( hex_str.c_str(), hex_str.length(), &array[0], &array_len); ASSERT_EQ(result, -1); + // Verify buffer wasn't modified + for (const auto &byte : array) { + EXPECT_EQ(byte, 0xAA); + }
298-309
: Consider validating output buffer integrity.Similar to the empty string test, it would be more thorough to verify that the output buffer remains unmodified when the conversion fails.
Add these assertions:
std::array<uint8_t, 2> array; + array.fill(0xAA); // Initialize with known pattern size_t array_len = array.size(); int result = oc_conv_hex_string_to_byte_array( hex_str.c_str(), hex_str.length(), &array[0], &array_len); ASSERT_EQ(result, -1); + // Verify buffer wasn't modified + for (const auto &byte : array) { + EXPECT_EQ(byte, 0xAA); + }
327-341
: Add documentation for odd-length handling.While the test is well-designed, it would be helpful to add a comment explaining how odd-length hex strings are handled (e.g., if leading zeros are assumed).
Add this documentation:
- // Test successful conversion of an odd-length hex string to byte array + // Test successful conversion of an odd-length hex string to byte array. + // For odd-length strings like "fa05bf", each pair of hex digits is converted + // to a byte, and the conversion should succeed without requiring leading zeros + // (i.e., "fa05bf" is treated the same as "0fa05bf"). TEST(Helpers, OddLengthHexStringSuccess)include/oc_network_monitor.h (2)
Line range hint
19-20
: Align header guard with filenameThe header guard
OC_NETWORK_HELPERS_H
doesn't match the filenameoc_network_monitor.h
. Consider renaming it toOC_NETWORK_MONITOR_H
for consistency.-#ifndef OC_NETWORK_HELPERS_H -#define OC_NETWORK_HELPERS_H +#ifndef OC_NETWORK_MONITOR_H +#define OC_NETWORK_MONITOR_H
Line range hint
33-49
: Enhance API documentationThe documentation is good but could be improved with additional details:
- Thread-safety guarantees
- Specific error conditions (e.g., out of memory, duplicate callback)
- Expected callback signature and lifecycle
Example enhancement for the first function:
/** * @brief Add the callback to receive change notifications for Network interface * event. + * + * The callback will be invoked when network interface changes occur (e.g., interface + * up/down, IP address changes). Multiple callbacks can be registered. + * * @param cb The callback to be added. Must not be NULL. + * Signature: void (*interface_event_handler_t)(oc_interface_event_t event) + * * @return 0 on success - * @return -1 on error + * @return -1 if cb is NULL, callback already registered, or out of memory + * + * @note Thread-safe: This function can be called from any thread */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
api/cloud/oc_cloud.c
(1 hunks)api/cloud/unittest/cloud_store_test.cpp
(1 hunks)api/oc_buffer_settings.c
(1 hunks)api/oc_discovery.c
(2 hunks)api/oc_helpers.c
(4 hunks)api/oc_main.c
(0 hunks)api/oc_session_events.c
(0 hunks)api/plgd/device-provisioning-client/plgd_dps.c
(2 hunks)api/plgd/device-provisioning-client/plgd_dps_manager.c
(0 hunks)api/plgd/unittest/plgdtimetest.cpp
(0 hunks)api/unittest/buffersettingstest.cpp
(1 hunks)api/unittest/helperstest.cpp
(1 hunks)include/oc_buffer_settings.h
(3 hunks)include/oc_enums.h
(5 hunks)include/oc_network_monitor.h
(2 hunks)include/oc_session_events.h
(2 hunks)include/oc_signal_event_loop.h
(1 hunks)include/oc_sp.h
(2 hunks)port/android/ipadapter.c
(1 hunks)port/esp32/adapter/src/ipadapter.c
(1 hunks)port/esp32/main/CMakeLists.txt
(1 hunks)port/linux/ipadapter.c
(1 hunks)port/oc_connectivity.h
(1 hunks)port/oc_dns.h
(1 hunks)port/oc_storage.h
(2 hunks)port/unittest/connectivitytest.cpp
(2 hunks)port/windows/ipadapter.c
(1 hunks)port/windows/vs2015/IoTivity-lite.vcxproj
(2 hunks)port/windows/vs2015/IoTivity-lite.vcxproj.filters
(2 hunks)swig/swig_interfaces/oc_buffer_settings.i
(1 hunks)swig/swig_interfaces/oc_enums.i
(1 hunks)swig/swig_interfaces/oc_session_events.i
(1 hunks)
💤 Files with no reviewable changes (4)
- api/oc_session_events.c
- api/plgd/device-provisioning-client/plgd_dps_manager.c
- api/plgd/unittest/plgdtimetest.cpp
- api/oc_main.c
✅ Files skipped from review due to trivial changes (1)
- swig/swig_interfaces/oc_session_events.i
🚧 Files skipped from review as they are similar to previous changes (23)
- api/cloud/unittest/cloud_store_test.cpp
- api/cloud/oc_cloud.c
- port/esp32/main/CMakeLists.txt
- swig/swig_interfaces/oc_buffer_settings.i
- swig/swig_interfaces/oc_enums.i
- include/oc_sp.h
- api/oc_helpers.c
- port/windows/ipadapter.c
- port/windows/vs2015/IoTivity-lite.vcxproj.filters
- api/oc_discovery.c
- port/oc_connectivity.h
- port/oc_dns.h
- include/oc_signal_event_loop.h
- port/oc_storage.h
- api/plgd/device-provisioning-client/plgd_dps.c
- port/android/ipadapter.c
- include/oc_enums.h
- api/oc_buffer_settings.c
- port/unittest/connectivitytest.cpp
- include/oc_buffer_settings.h
- api/unittest/buffersettingstest.cpp
- include/oc_session_events.h
- port/esp32/adapter/src/ipadapter.c
⏰ Context from checks skipped due to timeout of 90000ms (70)
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable, Release, -DOC_DISCOVERY_RESOUR... / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_UBSAN_ENABLED=ON, docker/apps/Dockerfile.dps-cloud-ser... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable-debug, Debug, -DOC_DISCOVERY_RE... / build-and-push-image
- GitHub Check: plgd-hub-test (cloud-server-asan-access-in-RFOTM, -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RF... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-faketime-system-time, --set-system-time, -DPLGD_DPS_FAKETIME_ENAB... / plgd-hub-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable, Release, -DOC_DISCOVERY_RESOUR... / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_UBSAN_ENABLED=ON, docker/apps/Dockerfile.dps-cloud-ser... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable-debug, Debug, -DOC_DISCOVERY_RE... / build-and-push-image
- GitHub Check: plgd-hub-test (cloud-server-asan-access-in-RFOTM, -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RF... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-faketime-system-time, --set-system-time, -DPLGD_DPS_FAKETIME_ENAB... / plgd-hub-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable, Release, -DOC_DISCOVERY_RESOUR... / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_UBSAN_ENABLED=ON, docker/apps/Dockerfile.dps-cloud-ser... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable-debug, Debug, -DOC_DISCOVERY_RE... / build-and-push-image
- GitHub Check: plgd-hub-test (cloud-server-asan-access-in-RFOTM, -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RF... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-faketime-system-time, --set-system-time, -DPLGD_DPS_FAKETIME_ENAB... / plgd-hub-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable, Release, -DOC_DISCOVERY_RESOUR... / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_UBSAN_ENABLED=ON, docker/apps/Dockerfile.dps-cloud-ser... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable-debug, Debug, -DOC_DISCOVERY_RE... / build-and-push-image
- GitHub Check: plgd-hub-test (cloud-server-asan-access-in-RFOTM, -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RF... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-faketime-system-time, --set-system-time, -DPLGD_DPS_FAKETIME_ENAB... / plgd-hub-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable, Release, -DOC_DISCOVERY_RESOUR... / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_UBSAN_ENABLED=ON, docker/apps/Dockerfile.dps-cloud-ser... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable-debug, Debug, -DOC_DISCOVERY_RE... / build-and-push-image
- GitHub Check: plgd-hub-test (cloud-server-asan-access-in-RFOTM, -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RF... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-faketime-system-time, --set-system-time, -DPLGD_DPS_FAKETIME_ENAB... / plgd-hub-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable, Release, -DOC_DISCOVERY_RESOUR... / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_UBSAN_ENABLED=ON, docker/apps/Dockerfile.dps-cloud-ser... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable-debug, Debug, -DOC_DISCOVERY_RE... / build-and-push-image
- GitHub Check: plgd-hub-test (cloud-server-asan-access-in-RFOTM, -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RF... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-faketime-system-time, --set-system-time, -DPLGD_DPS_FAKETIME_ENAB... / plgd-hub-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable, Release, -DOC_DISCOVERY_RESOUR... / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_UBSAN_ENABLED=ON, docker/apps/Dockerfile.dps-cloud-ser... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable-debug, Debug, -DOC_DISCOVERY_RE... / build-and-push-image
- GitHub Check: plgd-hub-test (cloud-server-asan-access-in-RFOTM, -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RF... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-faketime-system-time, --set-system-time, -DPLGD_DPS_FAKETIME_ENAB... / plgd-hub-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable, Release, -DOC_DISCOVERY_RESOUR... / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_UBSAN_ENABLED=ON, docker/apps/Dockerfile.dps-cloud-ser... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable-debug, Debug, -DOC_DISCOVERY_RE... / build-and-push-image
- GitHub Check: plgd-hub-test (cloud-server-asan-access-in-RFOTM, -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RF... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-faketime-system-time, --set-system-time, -DPLGD_DPS_FAKETIME_ENAB... / plgd-hub-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable, Release, -DOC_DISCOVERY_RESOUR... / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_UBSAN_ENABLED=ON, docker/apps/Dockerfile.dps-cloud-ser... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable-debug, Debug, -DOC_DISCOVERY_RE... / build-and-push-image
- GitHub Check: plgd-hub-test (cloud-server-asan-access-in-RFOTM, -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RF... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-faketime-system-time, --set-system-time, -DPLGD_DPS_FAKETIME_ENAB... / plgd-hub-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable, Release, -DOC_DISCOVERY_RESOUR... / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_UBSAN_ENABLED=ON, docker/apps/Dockerfile.dps-cloud-ser... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server-discovery-resource-observable-debug, Debug, -DOC_DISCOVERY_RE... / build-and-push-image
- GitHub Check: plgd-hub-test (cloud-server-asan-access-in-RFOTM, -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RF... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: plgd-hub-test (dps-cloud-server-faketime-system-time, --set-system-time, -DPLGD_DPS_FAKETIME_ENAB... / plgd-hub-test-with-cfg
🔇 Additional comments (6)
port/windows/vs2015/IoTivity-lite.vcxproj (1)
325-325
: LGTM! Project configuration updated correctly.The new files have been properly added to the Visual Studio project:
- Header files:
oc_buffer_settings.h
andoc_dns.h
- Source file:
oc_buffer_settings.c
Also applies to: 358-358
port/linux/ipadapter.c (2)
43-45
: Clean and well-structured conditional inclusion.The conditional inclusion of the network monitor header follows best practices and ensures proper modularization.
Line range hint
89-98
: Well-structured interface management data structures.The implementation uses OC_LIST and OC_MEMB macros consistently for memory management, and the structure is clean and focused.
api/unittest/helperstest.cpp (2)
260-270
: LGTM! Well-designed error case test.The test properly validates buffer overflow protection by ensuring the conversion fails when the destination buffer is too small to hold the hex string representation.
311-325
: LGTM! Comprehensive test for even-length hex string conversion.The test thoroughly validates the successful conversion path by checking the return code, output length, and the entire content of the output buffer.
include/oc_network_monitor.h (1)
Line range hint
1-53
: Verify removal of session event callbacksThe removal of session event callback functions (
oc_add_session_event_callback
,oc_remove_session_event_callback
, etc.) might impact existing code. Let's verify there are no remaining references:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CMakeLists.txt (2)
14-30
: Enhance robustness of linker detection.The linker detection mechanism could be improved in the following ways:
- Add version-specific handling for different linker versions
- Make the string matching more robust by using more specific patterns
Apply this diff to improve the linker detection:
execute_process( COMMAND ${CMAKE_LINKER} --version OUTPUT_VARIABLE LINKER_VERSION RESULT_VARIABLE LINKER_RESULT ERROR_QUIET ) set(OC_LINKER_IS_LD OFF CACHE BOOL "Linker is ld." FORCE) set(OC_LINKER_IS_LDD OFF CACHE BOOL "Linker is ldd." FORCE) if(LINKER_RESULT EQUAL 0) string(TOLOWER "${LINKER_VERSION}" LINKER_VERSION) - if(LINKER_VERSION MATCHES "ldd") + if(LINKER_VERSION MATCHES "^.*ldd.*version ([0-9]+\\.[0-9]+)") + set(LINKER_VERSION_STRING ${CMAKE_MATCH_1}) set(OC_LINKER_IS_LDD ON CACHE BOOL "Linker is ldd." FORCE) - elseif(LINKER_VERSION MATCHES "ld") + elseif(LINKER_VERSION MATCHES "^.*ld.*version ([0-9]+\\.[0-9]+)") + set(LINKER_VERSION_STRING ${CMAKE_MATCH_1}) set(OC_LINKER_IS_LD ON CACHE BOOL "Linker is ld." FORCE) endif() endif()
241-241
: Document the purpose of error suppression flags.While the flags are correctly applied, it would be helpful to document why these specific warnings are being suppressed for Windows builds.
Add comments explaining the rationale:
- oc_add_compile_options(GLOBAL FLAGS -Wno-error=gnu-offsetof-extensions -Wno-error=c23-extensions -Wno-error=sign-compare) + # Suppress Windows-specific warnings: + # - gnu-offsetof-extensions: MinGW uses GNU offsetof macro + # - c23-extensions: Windows headers may use C23 features + # - sign-compare: Windows APIs often mix signed/unsigned types + oc_add_compile_options(GLOBAL FLAGS + -Wno-error=gnu-offsetof-extensions # Allow GNU offsetof extension + -Wno-error=c23-extensions # Allow C23 features in Windows headers + -Wno-error=sign-compare # Allow signed/unsigned comparisons + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CMakeLists.txt
(3 hunks)
🔇 Additional comments (1)
CMakeLists.txt (1)
176-187
: Well-structured linker-specific optimizations.The compilation and linking flags are well-chosen and properly documented for both linker types. The mutually exclusive conditions ensure correct flag selection.
Quality Gate passedIssues Measures |
No description provided.